| Message ID | 20231122122715.2561213-17-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Tidy up file permission hooks | expand |
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@gmail.com> 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 >
On Fri, Nov 24, 2023 at 6:17 AM Jan Kara <jack@suse.cz> 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@gmail.com> > > 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@suse.com> > SUSE Labs, CR
On Fri 24-11-23 10:20:25, Amir Goldstein wrote: > On Fri, Nov 24, 2023 at 6:17 AM Jan Kara <jack@suse.cz> 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@gmail.com> > > > > 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
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
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@gmail.com> --- include/linux/fs.h | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+)