Re: [git pull] vfs.git part 1

7 messages, 3 authors, 2017-07-07 · open the first message on its own page

Re: [git pull] vfs.git part 1

From: Michael Ellerman <mpe@ellerman.id.au>
Date: 2017-07-07 12:46:32

Al Viro [off-list ref] writes:

	vfs.git topology is rather convoluted this cycle, so
I'm afraid that it'll take more pull requests than usual ;-/

The first pile is #work.misc-set_fs.  Assorted getting rid
of cargo-culted access_ok(), cargo-culted set_fs() and
field-by-field copyouts.  The same description applies to
a lot of stuff in other branches - this is just the stuff that
didn't fit into a more specific topical branch.

The following changes since commit c86daad2c25bfd4a33d48b7691afaa96d9c5ab46:

  Merge branch 'for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input (2017-05-26 16:45:13 -0700)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git work.misc-set_fs

for you to fetch changes up to 8c6657cb50cb037ff58b3f6a547c6569568f3527:

  Switch flock copyin/copyout primitives to copy_{from,to}_user() (2017-06-26 23:52:44 -0400)

This commit seems to have broken networking on a bunch of my PPC
machines (64-bit kernel, 32-bit userspace).

# first bad commit: [8c6657cb50cb037ff58b3f6a547c6569568f3527] Switch flock copyin/copyout primitives to copy_{from,to}_user()

The symptom is eth0 doesn't get address via dhcp.

Reverting it on top of master (9f45efb928) everything works OK again.


Trying to bring networking up manually gives:

# ifup eth0
ifup: failed to lock lockfile /run/network/ifstate.eth0: Invalid argument

strace shows:

5647  fcntl64(3, F_SETLK, {l_type=F_WRLCK, l_whence=SEEK_SET, l_start=0, l_len=0}) = -1 EINVAL (Invalid argument)
5647  write(2, "ifup: failed to lock lockfile /r"..., 74) = 74

vs the working case:

6005  fcntl64(3, F_SETLK, {l_type=F_WRLCK, l_whence=SEEK_SET, l_start=0, l_len=0}) = 0

Patch coming.

cheers

Re: [git pull] vfs.git part 1

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: 2017-07-07 15:59:46

On Fri, Jul 7, 2017 at 5:46 AM, Michael Ellerman [off-list ref] wrote:

Al Viro [off-list ref] writes:

quoted

  Switch flock copyin/copyout primitives to copy_{from,to}_user() (2017-06-26 23:52:44 -0400)

This commit seems to have broken networking on a bunch of my PPC
machines (64-bit kernel, 32-bit userspace).

Bah. I think that commit is entirely broken, due to having the
arguments to the "copy_flock_fields()" in the wrong order.

The copy_flock_fields() macro has the arguments in order <from, to>,
but all the users seem to do it the other way around.

I think it would have been more obvious if the put_compat_flock*()
source argument had been "const".

Patch coming.

I'm not seeing a patch, so I did my own. But it's _entirely_ untested.
Does the attached fix things for you?

                     Linus

Re: [git pull] vfs.git part 1

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: 2017-07-07 16:30:03

On Fri, Jul 7, 2017 at 8:59 AM, Linus Torvalds
[off-list ref] wrote:

quoted

Patch coming.

I'm not seeing a patch, so I did my own. But it's _entirely_ untested.
Does the attached fix things for you?

Oh, I see you sent a patch to the list but didn't cc me like in this thread.

Hmm. Al - I'd like to add the "const" parts at least. How the ordering
gets fixed (I changed it in the users of the macro, Michael changed
the macro itself) I don't much care about.

Can you get me a pull request soon since this presumably also breaks
every other compat case, and it just happened that power was the one
that noticed it first.. Or I can just commit my version, but I guess
Michael's is at least tested..

               Linus

Re: [git pull] vfs.git part 1

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: 2017-07-07 17:35:41

On Fri, Jul 7, 2017 at 8:59 AM, Linus Torvalds
[off-list ref] 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?

              Linus

Re: [git pull] vfs.git part 1

From: Al Viro <viro@ZenIV.linux.org.uk>
Date: 2017-07-07 18:59:58

On Fri, Jul 07, 2017 at 10:35:41AM -0700, Linus Torvalds wrote:

Comments? Michael, does this work for your case?

Looks sane...

quoted hunk

+++ 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;

Re: [git pull] vfs.git part 1

From: Michael Ellerman <mpe@ellerman.id.au>
Date: 2017-07-07 22:50:40

Linus Torvalds [off-list ref] writes:

On Fri, Jul 7, 2017 at 8:59 AM, Linus Torvalds
[off-list ref] wrote:

quoted

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

Re: [git pull] vfs.git part 1

From: Michael Ellerman <mpe@ellerman.id.au>
Date: 2017-07-07 22:55:14

Linus Torvalds [off-list ref] writes:

On Fri, Jul 7, 2017 at 8:59 AM, Linus Torvalds
[off-list ref] wrote:

quoted

Patch coming.

I'm not seeing a patch, so I did my own. But it's _entirely_ untested.
Does the attached fix things for you?

Oh, I see you sent a patch to the list but didn't cc me like in this thread.

Oops, I sent it To you, but I forgot to make it a reply to this thread
which was daft.

cheers

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help