| Message ID | 1380834638-24035-2-git-send-email-sebastian.capella@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On Thu, 3 Oct 2013 14:10:37 -0700 Sebastian Capella <sebastian.capella@linaro.org> wrote: > Enhance name_to_dev_t to handle trailing newline characters > on device paths. Some inputs to name_to_dev_t may come from > userspace where oftentimes a '\n' is appended to the path. > Added const to the name buffer in both the function > declaration and the prototype to reflect input buffer > handling. > > By handling trailing newlines in name_to_dev_t, userspace > buffers may be directly passed to name_to_dev_t without > modification. We have lib/string.c:strim() - perhaps this patch would be neater if it were to use it? -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Sebastian Capella <sebastian.capella@linaro.org> writes: > Quoting Sebastian Capella (2013-10-03 16:47:35) >> Quoting Sebastian Capella (2013-10-03 14:42:46) >> > Quoting Andrew Morton (2013-10-03 14:15:23) >> > > On Thu, 3 Oct 2013 14:10:37 -0700 Sebastian Capella <sebastian.capella@linaro.org> wrote: >> > > >> > > > Enhance name_to_dev_t to handle trailing newline characters >> > > > on device paths. Some inputs to name_to_dev_t may come from >> > > > userspace where oftentimes a '\n' is appended to the path. >> > > > Added const to the name buffer in both the function >> > > > declaration and the prototype to reflect input buffer >> > > > handling. >> > > > >> > > > By handling trailing newlines in name_to_dev_t, userspace >> > > > buffers may be directly passed to name_to_dev_t without >> > > > modification. >> > > >> > > We have lib/string.c:strim() - perhaps this patch would be >> > > neater if it were to use it? >> > >> > Hi Morton, >> > >> > I was intending to respect the const handling of the input buffer. >> > >> > The actual buffer in this case is not really const as it comes from >> > the file buffering, but removing the const requires changing the >> > store function defined in the kobj_attribute, and would propagate >> > to many areas in the kernel. >> > >> > Modifying the buffer and removing the const was also suggested by Pavel. >> > After some discussion I posted this version which did not change the >> > buffer or the prototype. >> > >> > Please let me know if the preference is to modify the store function >> > definition. >> > >> > I'll prepare a patchset that removes the consts to see how much is >> > changed. >> > >> > Thanks, >> > >> > Sebastian >> >> Hi Andrew, >> >> Sorry for calling you Morton earlier. >> >> I looked into removing the const from the store function, but I'm not sure >> this is the right idea, so I'm going to shelf that for now. >> >> Please let me know your thoughts. >> >> Thanks, >> >> Sebastian > > Ping... > > Hi Andrew, > > Do you have any feedback on this? > > Below are the three options considered thus far. Do > you have any additional suggestions or preferences? What is wrong with requiring userspace to use echo -n ? That by far seems the simplest and least error prone solution. Eric -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 28 Jan 2014 10:59:26 -0800 Sebastian Capella <sebastian.capella@linaro.org> wrote: > > Do you have any feedback for me on this? > > > > I'm happy do make any changes you think are correct, but I'm unsure if > > you're asking me for option #3 above. It's quite an intrusive change, > > and changes old, established code and I'd like confirmation that's what > > you'd like before proceeding down that path. > > > > I've submitted patches with both options #1 and #2 above. > > > > Thanks, > > > > Sebastian > > Ping. > > Sorry for the lapse in attention to this. > > Could you please clarify what is needed for this to be acceptable? > I'm a little confused about what is being asked of me. The problem is that kernel/power/hibernate.c:resume_store() is handed a newline-terminated string, yes? And if it blindly hands that string over to name_to_dev_t(), name_to_dev_t() fails because the string is wrong. This is an oddity of the sysfs->kernel interface and altering name_to_dev_t doesn't really seem appropriate for this problem - it would be better to fix the caller to pass in the correct string. Something like... /* * Clean up a string which may have leading and/or trailing whitespace (as * defined by isspace()) by trimming off that whitespace. Returns an address * which the caller must kfree(), or NULL on error. */ char *strim_copy(const char *s, gfp_t gfp) { char *ret = kstrdup(skip_spaces(s), gfp); if (ret) strim(ret); return ret; } EXPORT_SYMBOL(strim_copy); -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 29 Jan 2014 10:29:56 -0800 Sebastian Capella <sebastian.capella@linaro.org> wrote: > Hi Andrew, > > By the way, I do see a call (sysfs_streq) in use for this purpose > other places. Sorry, I didn't find it while looking at the original > problem. I'm not sure if this is preferable, but it appears to have > been added specifically for the strings coming through sysfs. Yes, I wrote it ;) I didn't think sysfs_streq() is well suited to this problem. And the issue of possibly-null-terminated-strings coming in from userspace is a common one, so it is desirable that we build up the suite of utilities to handle this. There are probably quite a lot of open-coded \n trimming loops which can be cleaned up using such tools. grep -r "if .* == '\\\n'" . > My preference is copying the string and cleaning it up before passing > it to internal functions, even though we incur an allocation. Yes. Here on the kernel/userspace boundary we are typically running in GFP_KERNEL context and the code is not performance critical - it is a good fit. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/linux/mount.h b/include/linux/mount.h index 38cd98f..fdbb3e6 100644 --- a/include/linux/mount.h +++ b/include/linux/mount.h @@ -77,6 +77,6 @@ extern struct vfsmount *vfs_kern_mount(struct file_system_type *type, extern void mnt_set_expiry(struct vfsmount *mnt, struct list_head *expiry_list); extern void mark_mounts_for_expiry(struct list_head *mounts); -extern dev_t name_to_dev_t(char *name); +extern dev_t name_to_dev_t(const char *name); #endif /* _LINUX_MOUNT_H */ diff --git a/init/do_mounts.c b/init/do_mounts.c index a51cddc..69d74ff 100644 --- a/init/do_mounts.c +++ b/init/do_mounts.c @@ -145,6 +145,13 @@ static dev_t devt_from_partuuid(const char *uuid_str) clear_root_wait = true; goto done; } + if (uuid_str[cmp.len - 1] == '\n') { + cmp.len--; + if (!cmp.len) { + clear_root_wait = true; + goto done; + } + } dev = class_find_device(&block_class, NULL, &cmp, &match_dev_by_uuid); @@ -204,12 +211,13 @@ done: * bangs. */ -dev_t name_to_dev_t(char *name) +dev_t name_to_dev_t(const char *name) { char s[32]; char *p; dev_t res = 0; int part; + int n; #ifdef CONFIG_BLOCK if (strncmp(name, "PARTUUID=", 9) == 0) { @@ -230,7 +238,7 @@ dev_t name_to_dev_t(char *name) goto fail; } else { res = new_decode_dev(simple_strtoul(name, &p, 16)); - if (*p) + if (*p && *p != '\n') goto fail; } goto done; @@ -238,15 +246,20 @@ dev_t name_to_dev_t(char *name) name += 5; res = Root_NFS; - if (strcmp(name, "nfs") == 0) + if (strncmp(name, "nfs", 3) == 0) goto done; res = Root_RAM0; - if (strcmp(name, "ram") == 0) + if (strncmp(name, "ram", 3) == 0) goto done; - if (strlen(name) > 31) + n = strlen(name); + if (n != 0 && name[n - 1] == '\n') + n--; + if (n > 31) goto fail; - strcpy(s, name); + strncpy(s, name, n); + s[n] = '\0'; + for (p = s; *p; p++) if (*p == '/') *p = '!';