On Fri 24-11-23 10:20:25, Amir Goldstein wrote:
> 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.
Yeah, fair enough. Thanks for explanation!
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
