| Message ID | CA+55aFzHBR9M+UN2CMAQ0np275iyWyAY0uKK8LFo33g0e7Z-VQ@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jul 07, 2017 at 10:35:41AM -0700, Linus Torvalds wrote: > Comments? Michael, does this work for your case? Looks sane... > +++ b/fs/fcntl.c > @@ -520,50 +520,50 @@ SYSCALL_DEFINE3(fcntl64, unsigned int, fd, unsigned int, cmd, > > #ifdef CONFIG_COMPAT > /* careful - don't use anywhere else */ > -#define copy_flock_fields(from, to) \ > - (to).l_type = (from).l_type; \ > - (to).l_whence = (from).l_whence; \ > - (to).l_start = (from).l_start; \ > - (to).l_len = (from).l_len; \ > - (to).l_pid = (from).l_pid; > - > -static int get_compat_flock(struct flock *kfl, struct compat_flock __user *ufl) > +#define copy_flock_fields(dst, src) \ > + (dst)->l_type = (src)->l_type; \ > + (dst)->l_whence = (src)->l_whence; \ > + (dst)->l_start = (src)->l_start; \ > + (dst)->l_len = (src)->l_len; \ > + (dst)->l_pid = (src)->l_pid; > + > +static int get_compat_flock(struct flock *kfl, const struct compat_flock __user *ufl) > { > struct compat_flock fl; > > if (copy_from_user(&fl, ufl, sizeof(struct compat_flock))) > return -EFAULT; > - copy_flock_fields(*kfl, fl); > + copy_flock_fields(kfl, &fl); > return 0; > } > > -static int get_compat_flock64(struct flock *kfl, struct compat_flock64 __user *ufl) > +static int get_compat_flock64(struct flock *kfl, const struct compat_flock64 __user *ufl) > { > struct compat_flock64 fl; > > if (copy_from_user(&fl, ufl, sizeof(struct compat_flock64))) > return -EFAULT; > - copy_flock_fields(*kfl, fl); > + copy_flock_fields(kfl, &fl); > return 0; > } > > -static int put_compat_flock(struct flock *kfl, struct compat_flock __user *ufl) > +static int put_compat_flock(const struct flock *kfl, struct compat_flock __user *ufl) > { > struct compat_flock fl; > > memset(&fl, 0, sizeof(struct compat_flock)); > - copy_flock_fields(fl, *kfl); > + copy_flock_fields(&fl, kfl); > if (copy_to_user(ufl, &fl, sizeof(struct compat_flock))) > return -EFAULT; > return 0; > } > > -static int put_compat_flock64(struct flock *kfl, struct compat_flock64 __user *ufl) > +static int put_compat_flock64(const struct flock *kfl, struct compat_flock64 __user *ufl) > { > struct compat_flock64 fl; > > memset(&fl, 0, sizeof(struct compat_flock64)); > - copy_flock_fields(fl, *kfl); > + copy_flock_fields(&fl, kfl); > if (copy_to_user(ufl, &fl, sizeof(struct compat_flock64))) > return -EFAULT; > return 0;
Linus Torvalds <torvalds@linux-foundation.org> writes: > On Fri, Jul 7, 2017 at 8:59 AM, Linus Torvalds > <torvalds@linux-foundation.org> wrote: >> >> The copy_flock_fields() macro has the arguments in order <from, to>, >> but all the users seem to do it the other way around. > > Looking more at it, I think I'd also like copy_flock_fields() to take > pointer arguments, to match all the code around it (both > copy_to/from_user and the memset calls. > > The actual order of arguments I suspect Michael's patch did better - > make the copy_flock_fields() just match the order of memcpy() and > copy_to/from_user(), both of which have <dest,src> order. > > So I think my preferred patch would be something like this, even if it > is bigger than either. > > Comments? Michael, does this work for your case? Yeah that works, as committed in your tree. Sorry for the slow reply, our time zones don't line up all that well :) cheers
diff --git a/fs/fcntl.c b/fs/fcntl.c index b6bd89628025..3b01b646e528 100644 --- a/fs/fcntl.c +++ b/fs/fcntl.c @@ -520,50 +520,50 @@ SYSCALL_DEFINE3(fcntl64, unsigned int, fd, unsigned int, cmd, #ifdef CONFIG_COMPAT /* careful - don't use anywhere else */ -#define copy_flock_fields(from, to) \ - (to).l_type = (from).l_type; \ - (to).l_whence = (from).l_whence; \ - (to).l_start = (from).l_start; \ - (to).l_len = (from).l_len; \ - (to).l_pid = (from).l_pid; - -static int get_compat_flock(struct flock *kfl, struct compat_flock __user *ufl) +#define copy_flock_fields(dst, src) \ + (dst)->l_type = (src)->l_type; \ + (dst)->l_whence = (src)->l_whence; \ + (dst)->l_start = (src)->l_start; \ + (dst)->l_len = (src)->l_len; \ + (dst)->l_pid = (src)->l_pid; + +static int get_compat_flock(struct flock *kfl, const struct compat_flock __user *ufl) { struct compat_flock fl; if (copy_from_user(&fl, ufl, sizeof(struct compat_flock))) return -EFAULT; - copy_flock_fields(*kfl, fl); + copy_flock_fields(kfl, &fl); return 0; } -static int get_compat_flock64(struct flock *kfl, struct compat_flock64 __user *ufl) +static int get_compat_flock64(struct flock *kfl, const struct compat_flock64 __user *ufl) { struct compat_flock64 fl; if (copy_from_user(&fl, ufl, sizeof(struct compat_flock64))) return -EFAULT; - copy_flock_fields(*kfl, fl); + copy_flock_fields(kfl, &fl); return 0; } -static int put_compat_flock(struct flock *kfl, struct compat_flock __user *ufl) +static int put_compat_flock(const struct flock *kfl, struct compat_flock __user *ufl) { struct compat_flock fl; memset(&fl, 0, sizeof(struct compat_flock)); - copy_flock_fields(fl, *kfl); + copy_flock_fields(&fl, kfl); if (copy_to_user(ufl, &fl, sizeof(struct compat_flock))) return -EFAULT; return 0; } -static int put_compat_flock64(struct flock *kfl, struct compat_flock64 __user *ufl) +static int put_compat_flock64(const struct flock *kfl, struct compat_flock64 __user *ufl) { struct compat_flock64 fl; memset(&fl, 0, sizeof(struct compat_flock64)); - copy_flock_fields(fl, *kfl); + copy_flock_fields(&fl, kfl); if (copy_to_user(ufl, &fl, sizeof(struct compat_flock64))) return -EFAULT; return 0;