[v2,16/16] fs: create {sb,file}_write_not_started() helpers

Commit Message

Amir Goldstein Nov. 22, 2023, 12:27 p.m. UTC

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 <[e-mail]>
---
 include/linux/fs.h | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

Comments

Jan Kara Nov. 23, 2023, 5:35 p.m. UTC | #1

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 <[e-mail]>

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). Because one easily
forgets about the subtle implementation details and uses
!sb_write_started() instead of sb_write_not_started()...

								Honza

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

Amir Goldstein Nov. 24, 2023, 8:20 a.m. UTC | #2

On Fri, Nov 24, 2023 at 6:17 AM Jan Kara <[e-mail]> 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 <[e-mail]>
>
> 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 <[e-mail]>
> SUSE Labs, CR

Jan Kara Dec. 1, 2023, 10:11 a.m. UTC | #3

On Fri 24-11-23 10:20:25, Amir Goldstein wrote:
> On Fri, Nov 24, 2023 at 6:17 AM Jan Kara <[e-mail]> 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 <[e-mail]>
> >
> > 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.

Yeah, fair enough. Thanks for explanation!

								Honza

Patch

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