On Wed 19-06-19 16:04:04, Amir Goldstein wrote:
> On Wed, Jun 19, 2019 at 3:53 PM Jan Kara <jack@xxxxxxx> wrote:
> >
> > On Wed 19-06-19 13:34:44, Amir Goldstein wrote:
> > > When implementing connector fsid cache, we only initialized the cache
> > > when the first mark added to object was added by FAN_REPORT_FID group.
> > > We forgot to update conn->fsid when the second mark is added by
> > > FAN_REPORT_FID group to an already attached connector without fsid
> > > cache.
> > >
> > > Reported-and-tested-by: syzbot+c277e8e2f46414645508@xxxxxxxxxxxxxxxxxxxxxxxxx
> > > Fixes: 77115225acc6 ("fanotify: cache fsid in fsnotify_mark_connector")
> > > Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
> > > ---
> > >
> > > Jan,
> > >
> > > This fix has been confirmed by syzbot to fix the issue as well as
> > > by my modification to Matthew's LTP test:
> > > https://github.com/amir73il/ltp/commits/fanotify_dirent
> >
> > Thanks for the fix Amir. I somewhat don't like the additional flags field
> > (which ends up growing fsnotify_mark_connector by one long) for just that
> > one special flag. If nothing else, can't we just store the flag inside
> > 'type'? There's plenty of space there...
>
> I didn't think it mattered in the grand scheme of things, but
Well, the connector size usually isn't a huge concern but there can be lots
of connectors when someone watches large number of files so I prefer not to
waste space unnecessarily.
> I did consider:
> - unsigned int type; /* Type of object [lock] */
> + unsigned short type; /* Type of object [lock] */
> +#define FSNOTIFY_CONN_FLAG_HAS_FSID 0x01
> + unsigned short flags; /* flags [lock] */
>
> I think it makes sense.
> Let me know if you want me to resend of if you can fix on commit.
Yes, this is even less intrusive than what I had in mind. I'll apply this
change on commit. Thanks!
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
