On Fri, Nov 24, 2023 at 6:17 AM Jan Kara <jack@xxxxxxx> wrote:
>
> On Wed 22-11-23 14:27:15, Amir Goldstein wrote:
> > Create new helpers {sb,file}_write_not_started() that can be used
> > to assert that sb_start_write() is not held.
> >
> > This is needed for fanotify "pre content" events.
> >
> > Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
>
> I'm not against this but I'm somewhat wondering, where exactly do you plan
> to use this :) (does not seem to be in this patch set).
As I wrote in the cover letter:
"The last 3 patches are helpers that I used in fanotify patches to
assert that permission hooks are called with expected locking scope."
But this is just half of the story.
The full story is that I added it in fsnotify_file_perm() hook to check
that we caught all the places that permission hook was called with
sb_writers held:
static inline int fsnotify_file_perm(struct file *file, int mask)
{
struct inode *inode = file_inode(file);
__u32 fsnotify_mask;
/*
* Content of file may be written on pre-content events, so sb freeze
* protection must not be held.
*/
lockdep_assert_once(file_write_not_started(file));
/*
* Pre-content events are only reported for regular files and dirs.
*/
if (mask & MAY_READ) {
And the assert triggered in a nested overlay case (overlay over overlay).
So I cannot keep the assert in the final patch as is.
I can probably move it into (mask & MAY_WRITE) case, because
I don't know of any existing write permission hook that is called with
sb_wrtiers held.
I also plan to use sb_write_not_started() in fsnotify_lookup_perm().
I think that:
"This is needed for fanotify "pre content" events."
sums this up nicely without getting into gory details ;)
> Because one easily
> forgets about the subtle implementation details and uses
> !sb_write_started() instead of sb_write_not_started()...
>
I think I had a comment in one version that said:
"This is NOT the same as !sb_write_started()"
We can add it back if you think it is useful, but FWIW, anyone
can use !sb_write_started() wrongly today whether we add
sb_write_not_started() or not.
But this would be pretty easy to detect - running a build without
CONFIG_LOCKDEP will catch this misuse pretty quickly.
Thanks,
Amir.
>
> > ---
> > include/linux/fs.h | 26 ++++++++++++++++++++++++++
> > 1 file changed, 26 insertions(+)
> >
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index 05780d993c7d..c085172f832f 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -1669,6 +1669,17 @@ static inline bool sb_write_started(const struct super_block *sb)
> > return __sb_write_started(sb, SB_FREEZE_WRITE);
> > }
> >
> > +/**
> > + * sb_write_not_started - check if SB_FREEZE_WRITE is not held
> > + * @sb: the super we write to
> > + *
> > + * May be false positive with !CONFIG_LOCKDEP/LOCK_STATE_UNKNOWN.
> > + */
> > +static inline bool sb_write_not_started(const struct super_block *sb)
> > +{
> > + return __sb_write_started(sb, SB_FREEZE_WRITE) <= 0;
> > +}
> > +
> > /**
> > * file_write_started - check if SB_FREEZE_WRITE is held
> > * @file: the file we write to
> > @@ -1684,6 +1695,21 @@ static inline bool file_write_started(const struct file *file)
> > return sb_write_started(file_inode(file)->i_sb);
> > }
> >
> > +/**
> > + * file_write_not_started - check if SB_FREEZE_WRITE is not held
> > + * @file: the file we write to
> > + *
> > + * May be false positive with !CONFIG_LOCKDEP/LOCK_STATE_UNKNOWN.
> > + * May be false positive with !S_ISREG, because file_start_write() has
> > + * no effect on !S_ISREG.
> > + */
> > +static inline bool file_write_not_started(const struct file *file)
> > +{
> > + if (!S_ISREG(file_inode(file)->i_mode))
> > + return true;
> > + return sb_write_not_started(file_inode(file)->i_sb);
> > +}
> > +
> > /**
> > * sb_end_write - drop write access to a superblock
> > * @sb: the super we wrote to
> > --
> > 2.34.1
> >
> --
> Jan Kara <jack@xxxxxxxx>
> SUSE Labs, CR
