| Message ID | 1456699822-2924-1-git-send-email-amber@thrall.me (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, Feb 28, 2016 at 11:03:03PM +0000, Al Viro wrote: > On Sun, Feb 28, 2016 at 02:50:22PM -0800, Amber Thrall wrote: > > The simple_strncmp_to_buffer() function provides an easier method for > > developers to compare a kernel space buffer against user space data. This > > process is done in a few drivers and may be simplified to a single function. > > *blink* > > The name is rather confusing and I would say that semantics is unexpected > as well. I would like to see proposed users of that primitive... BTW, such calling conventions are going to breed bugs - it's "0 if equal, something positive if greater, something negative if less, except when returned negative happens to be -EFAULT or -ENOMEM, in which cases it's an error". It would be _very_ easy to get wrong in callers. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/28, Al Viro wrote: > On Sun, Feb 28, 2016 at 02:50:22PM -0800, Amber Thrall wrote: > > The simple_strncmp_to_buffer() function provides an easier method for > > developers to compare a kernel space buffer against user space data. This > > process is done in a few drivers and may be simplified to a single function. > > *blink* > > The name is rather confusing and I would say that semantics is unexpected > as well. I would like to see proposed users of that primitive... Apologies for the confusing name, struggled to find an appropriate name while staying consistent with the naming schemes of simple_read/write_to_buffer() functions, as it based off of them. I'd love to hear alternative names. I saw possible uses for this proposed function being an easy way to interact with debugfs, via their write file operation. For example in the function xenvif_write_io_ring() the string "kick" is checked for against a user space buffer. Thanks for the fast reply. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Feb 28, 2016 at 03:39:08PM -0800, Amber Thrall wrote: > Apologies for the confusing name, struggled to find an appropriate name > while staying consistent with the naming schemes of > simple_read/write_to_buffer() functions, as it based off of them. I'd > love to hear alternative names. > > I saw possible uses for this proposed function being an easy way to > interact with debugfs, via their write file operation. For > example in the function xenvif_write_io_ring() the string "kick" is > checked for against a user space buffer. TBH, that caller leaves an impression of rather... poor API - "any write of no more than 32 bytes that starts with 'k' 'i' 'c' 'k' is OK (and everything beyond first 4 characters is ignored), anything else is rejected, in some cases with whining into syslog, in some - quietly". I don't know if encouraging stuff like that is a good idea... In any case, you've ended up open-coding kmemdup_user() + strncmp() + kfree(); the problem with combining those into a single helper is that calling conventions will be very error-prone - you have zero/positive/negative for passing strncmp() result *and* you need to report errors somehow. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/29, Al Viro wrote: > On Sun, Feb 28, 2016 at 03:39:08PM -0800, Amber Thrall wrote: > > Apologies for the confusing name, struggled to find an appropriate name > > while staying consistent with the naming schemes of > > simple_read/write_to_buffer() functions, as it based off of them. I'd > > love to hear alternative names. > > > > I saw possible uses for this proposed function being an easy way to > > interact with debugfs, via their write file operation. For > > example in the function xenvif_write_io_ring() the string "kick" is > > checked for against a user space buffer. > > TBH, that caller leaves an impression of rather... poor API - "any write > of no more than 32 bytes that starts with 'k' 'i' 'c' 'k' is OK (and > everything beyond first 4 characters is ignored), anything else is > rejected, in some cases with whining into syslog, in some - quietly". > I don't know if encouraging stuff like that is a good idea... > > In any case, you've ended up open-coding kmemdup_user() + strncmp() + kfree(); > the problem with combining those into a single helper is that calling > conventions will be very error-prone - you have zero/positive/negative for > passing strncmp() result *and* you need to report errors somehow. The conflicts between strncmp() and error values hadn't crossed my mind. The function could return the value from strncmp() via pointer, but it wouldn't match up with strncmp's formatting making the function confusing to use. Thanks for the input, I'm still new to working with the kernel and have a lot to learn. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" 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/fs/libfs.c b/fs/libfs.c index 0ca80b2..d588f2d 100644 --- a/fs/libfs.c +++ b/fs/libfs.c @@ -639,6 +639,39 @@ ssize_t simple_write_to_buffer(void *to, size_t available, loff_t *ppos, EXPORT_SYMBOL(simple_write_to_buffer); +/** + * simple_strncmp_to_buffer - compare data from user space to a buffer + * @to: the buffer to compare to + * @from: the user space buffer to read from + * @count: the maximum number of bytes to check + * + * The simple_strncmp_to_buffer() function compares a buffer and a user space + * buffer. This is similar to strncmp() but between kernel and user space + * buffers. + * + * On success, an integer less than, equal to, or greater than zero if @to (or + * the first @count bytes) is found, respectively, to be less than, to match, or + * be greater than @from. A negative value is returned on error. + **/ +int simple_strncmp_to_buffer(void *to, const void __user *from, size_t count) +{ + size_t res; + char *data; + + data = kmalloc(count + 1, GFP_KERNEL); + if (!data) + return -ENOMEM; + res = copy_from_user(data, from, count); + if (res) { + kfree(data); + return -EFAULT; + } + + res = strncmp(data, to, count); + kfree(data); + return res; +} +EXPORT_SYMBOL(simple_strncmp_to_buffer); + /** * memory_read_from_buffer - copy data from the buffer * @to: the kernel space buffer to read to * @count: the maximum number of bytes to read diff --git a/include/linux/fs.h b/include/linux/fs.h index 2a15fe2..c7eac75 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -2903,6 +2903,7 @@ extern ssize_t simple_read_from_buffer(void __user *to, size_t count, loff_t *ppos, const void *from, size_t available); extern ssize_t simple_write_to_buffer(void *to, size_t available, loff_t *ppos, const void __user *from, size_t count); +extern int simple_strncmp_to_buffer(void *to, const void __user *from, + size_t count); extern int __generic_file_fsync(struct file *, loff_t, loff_t, int); extern int generic_file_fsync(struct file *, loff_t, loff_t, int);
The simple_strncmp_to_buffer() function provides an easier method for developers to compare a kernel space buffer against user space data. This process is done in a few drivers and may be simplified to a single function. Signed-off-by: Amber Thrall <amber@thrall.me> --- fs/libfs.c | 33 +++++++++++++++++++++++++++++++++ include/linux/fs.h | 1 + 2 files changed, 34 insertions(+)