netdev - Re: [PATCH][CFT][experimental] net/socket.c: use straight fdget/fdput (resend)

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Mon, 27 May 2024 00:16:41 +0100
From: Al Viro <viro@...iv.linux.org.uk>
To: Linus Torvalds <torvalds@...ux-foundation.org>
Cc: netdev@...r.kernel.org, linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH][CFT][experimental] net/socket.c: use straight
 fdget/fdput (resend)

On Sun, May 26, 2024 at 03:16:37PM -0700, Linus Torvalds wrote:

> Hopefully in most cases the compiler sees the previous test for
> fd.file, realizes the new test is unnecessary and optimizes it away.

It does.

> Except we most definitely pass around 'struct fd *' in some places (at
> least ovlfs), so I doubt that  will be the case everywhere.

... and those places need care, really.  I've done that review
quite recently.  I don't think you'd been on Cc in related thread...
<checks> nope.

|| Another is overlayfs ovl_real_fdget() and ovl_real_fdget_meta().
|| Those two are arguably too clever for their readers' good.
|| It's still "borrow or clone, and remember which one had it been",
|| but that's mixed with returning errors:
||         err = ovl_read_fdget(file, &fd);
|| may construct and store a junk value in fd if err is non-zero.
|| Not a bug, but only because all callers remember to ignore that
|| value in such case.

Junk value as in e.g. <ERR_PTR(-EIO), FDPUT_FPUT>; it's trivial
to avoid (
		file = ovl_open_realfile(file, &realpath);
		if (IS_ERR(file))
			return ERR_PTR(file);
                real->flags = FDPUT_FPUT;
                real->file = file;
		return 0;
instead of
                real->flags = FDPUT_FPUT;
                real->file = ovl_open_realfile(file, &realpath);

                return PTR_ERR_OR_ZERO(real->file);
we have there right now and similar for <ERR_PTR(-EPERM), 0> pairs),
but note that anything like your switch to struct-wrapped unsigned long
would need similar massage there anyway.

> What would make more sense is if you make the "fd_empty()" test be
> about the _flags_, and then both the fp_empty() test and the test
> inside fdput() would be testing the same things.

Umm...  What encoding would you use?

> Sadly, we'd need another bit in the flags. One option is easy enough -
> we'd just have to make 'struct file' always be 8-byte aligned, which
> it effectively always is.
> 
> Or we'd need to make the rule be that FDPUT_POS_UNLOCK only gets set
> if FDPUT_FPUT is set.

Huh?  That makes no sense - open a file, fork, and there you go;
same file reference in unshared descriptor tables in child and parent.
You definitely don't want to bump refcount on fdget_pos() in either
and you don't want to lose read/write/lseek serialization.

FDPUT_FPUT is *not* a property of file; it's about the original
reference to file not being guaranteed to stay pinned for the lifetime
of struct fd in question...