From: Kent Overstreet <kent.overstreet@linux.dev>
To: Christian Brauner <brauner@kernel.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>,
linux-bcachefs@vger.kernel.org, linux-fsdevel@vger.kernel.org,
david@fromorbit.com, mcgrof@kernel.org, hch@lst.de,
willy@infradead.org
Subject: Re: [PATCH 2/2] bcachefs: Buffered write path now can avoid the inode lock
Date: Thu, 29 Feb 2024 11:43:27 -0500 [thread overview]
Message-ID: <y6hurdequpds6fgpsm3vlapzzfj6e5pgs2ukfjm4qbziv5kdwr@lrjkcnzkjh75> (raw)
In-Reply-To: <20240229-gestrafft-waage-157c1c4ee225@brauner>
On Thu, Feb 29, 2024 at 10:46:48AM +0100, Christian Brauner wrote:
> On Wed, Feb 28, 2024 at 11:27:05PM -0800, Linus Torvalds wrote:
> > On Wed, 28 Feb 2024 at 23:20, Linus Torvalds
> > <torvalds@linux-foundation.org> wrote:
> > >
> > >
> > > - take the lock exclusively if O_APPEND or if it *looks* like you
> > > might extend the file size.
> > >
> > > - otherwise, take the shared lock, and THEN RE-CHECK. The file size
> > > might have changed, so now you need to double-check that you're really
> > > not going to extend the size of the file, and if you are, you need to
> > > go back and take the inode lock exclusively after all.
> >
> > Same goes for the suid/sgid checks. You need to take the inode lock
> > shared to even have the i_mode be stable, and at that point you might
> > decide "oh, I need to clear suid/sgid after all" and have to go drop
> > the lock and retake it for exclusive access after all.
>
> I agree. And this is how we've done it in xfs as well. The inode lock is
> taken shared, then check for IS_NOSEC(inode) and if that's true keep the
> shared lock, otherwise upgrade to an exclusive lock. Note that this
> whole check also checks filesystem capability xattrs.
>
> I think you'd also get potential security issues or at least very subtle
> behavior. Someone could make a binary setuid and a concurrent write
> happens. chmod is taking the exclusive lock and there's a concurrent
> write going on changing the contents to something unsafe without being
> forced to remove the set*id bits.
>
> Similar for {g,u}id changes. Someone starts a chown() taking inode lock
> while someone issues a concurrent write. The chown changes the group
> ownership so that a writer would be forced to drop the sgid bit. But the
> writer proceeds keeping that bit.
That's the kind of race that's not actually a race - if the syscalls
being invoked at the same time, it's fine if the result is as if the
write happened first, then the chown.
For there to be an actual race one of two things would have to be the
case:
- the chown/chmod path would have to be dependent on the on the data
the write path is changing, or
- there'd have to be a problem wnith the write path seeing inconsistent
state mid modification
But I can't claim 100% the second case isn't a factor, I don't know the
security side of things as well, and the XFS way sounds nice and clean.
next prev parent reply other threads:[~2024-02-29 16:43 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-29 6:30 [PATCH 0/2] buffered write path without inode lock (for bcachefs) Kent Overstreet
2024-02-29 6:30 ` [PATCH 1/2] fs: file_remove_privs_flags() Kent Overstreet
2024-02-29 6:30 ` [PATCH 2/2] bcachefs: Buffered write path now can avoid the inode lock Kent Overstreet
2024-02-29 7:20 ` Linus Torvalds
2024-02-29 7:27 ` Linus Torvalds
2024-02-29 8:06 ` Kent Overstreet
2024-02-29 9:46 ` Christian Brauner
2024-02-29 16:43 ` Kent Overstreet [this message]
2024-02-29 7:42 ` Kent Overstreet
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=y6hurdequpds6fgpsm3vlapzzfj6e5pgs2ukfjm4qbziv5kdwr@lrjkcnzkjh75 \
--to=kent.overstreet@linux.dev \
--cc=brauner@kernel.org \
--cc=david@fromorbit.com \
--cc=hch@lst.de \
--cc=linux-bcachefs@vger.kernel.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=mcgrof@kernel.org \
--cc=torvalds@linux-foundation.org \
--cc=willy@infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).