Date:   Wed, 9 Sep 2020 19:03:07 +0300
From:   Amir Goldstein <amir73il@...il.com>
To:     Jan Kara <jack@...e.cz>
Cc:     Xiaoming Ni <nixiaoming@...wei.com>,
        Matthew Wilcox <willy@...radead.org>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        wangle6@...wei.com
Subject: Re: Question: Why is there no notification when a file is opened
 using filp_open()?

On Wed, Sep 9, 2020 at 2:11 PM Jan Kara <jack@...e.cz> wrote:
>
> On Wed 09-09-20 10:36:57, Amir Goldstein wrote:
> > On Wed, Sep 9, 2020 at 10:00 AM Xiaoming Ni <nixiaoming@...wei.com> wrote:
> > >
> > > On 2020/9/9 11:44, Amir Goldstein wrote:
> > > > On Tue, Sep 8, 2020 at 8:19 PM Matthew Wilcox <willy@...radead.org> wrote:
> > > >>
> > > >> On Tue, Sep 08, 2020 at 04:18:29PM +0300, Amir Goldstein wrote:
> > > >>> On Tue, Sep 8, 2020 at 3:53 PM Xiaoming Ni <nixiaoming@...wei.com> wrote:
> > > >>>> For example, in fs/coredump.c, do_coredump() calls filp_open() to
> > > >>>> generate core files.
> > > >>>> In this scenario, the fsnotify_open() notification is missing.
> > > >>>
> > > >>> I am not convinced that we should generate an event.
> > > >>> You will have to explain in what is the real world use case that requires this
> > > >>> event to be generated.
> > > >>
> > > >> Take the typical usage for fsnotify of a graphical file manager.
> > > >> It would be nice if the file manager showed a corefile as soon as it
> > > >> appeared in a directory rather than waiting until some other operation
> > > >> in that directory caused those directory contents to be refreshed.
> > > >
> > > > fsnotify_open() is not the correct notification for file managers IMO.
> > > > fsnotify_create() is and it will be called in this case.
> > > >
> > > > If the reason you are interested in open events is because you want
> > > > to monitor the entire filesystem then welcome to the future -
> > > > FAN_CREATE is supported since kernel v5.1.
> > > >
> > > > Is there another real life case you have in mind where you think users
> > > > should be able to get an open fd for a file that the kernel has opened?
> > > > Because that is what FAN_OPEN will do.
> > > >
> > >
> > > There are also cases where file is opened in read-only mode using
> > > filp_open().
> > >
> > > case1: nfsd4_init_recdir() call filp_open()
> > > filp_open()
> > > nfsd4_init_recdir() fs/nfsd/nfs4recover.c#L543
> > >
> > > L70: static char user_recovery_dirname[PATH_MAX] =
> > > "/var/lib/nfs/v4recovery";
> > > L543: nn->rec_file = filp_open(user_recovery_dirname, O_RDONLY |
> > > O_DIRECTORY, 0);
> > >
> > >
> > > case2: ima_read_policy()
> > > filp_open()
> > > kernel_read_file_from_path()  fs/exec.c#L1004
> > > ima_read_policy()  security/integrity/ima/ima_fs.c#L286
> > > ima_write_policy() security/integrity/ima/ima_fs.c#L335
> > > ima_measure_policy_ops   security/integrity/ima/ima_fs.c#L443
> > > sys_write()
> > >
> > > case3: use do_file_open_root() to open file
> > > do_file_open_root()
> > > file_open_root()   fs/open.c#L1159
> > > kernel_read_file_from_path_initns()  fs/exec.c#L1029
> > > fw_get_filesystem_firmware()  drivers/base/firmware_loader/main.c#L498
> > >
> > > Do we need to add fsnotify_open() in these scenarios?
> >
> > We do not *need* to add fsnotify_open() if there is no concrete use case
> > from real life that needs it.
> >
> > Matthew gave an example of a real life use case and I explained why IMO
> > we don't need to add fsnotify_open() for the use case that he described.
> >
> > If you want to add fsnotify_open() to any call site, please come up with
> > a real life use case - not a made up one, one that really exists and where
> > the open event is really needed.
> >
> > grepping the code for callers of filp_open() is not enough.
>
> Yeah. So in kernel, things are both ways. There are filp_open() users that
> do take care to manually generate fsnotify_open() event (most notably
> io_uring, exec, or do_handle_open) and there are others as Xiaoming found
> which just don't bother.  I'm not sure filp_open() should unconditionally
> generate fsnotify_open() event as IMO some of those notifications would be
> more confusing than useful.
>
> OTOH it is true that e.g. for core dumping we will generate other fsnotify
> events such as FSNOTIFY_CLOSE (which is generated in __fput()) so missing

And to be fair, those kernel callers will probably also end up generating
FS_ACCESS/FS_MODIFY too.

> FSNOTIFY_OPEN is somewhat confusing. So having some consistency in this
> (either by generating FSNOTIFY_OPEN or by not generating FSNOTIFY_CLOSE)
> would be IMO desirable.

Well, dropping events (FS_CLOSE in particular) didn't go down well the
last time we tried it:
https://lore.kernel.org/linux-fsdevel/CAOQ4uxg8E-im=B6L0PQNaTTKdtxVAO=MSJki7kxq875ME4hOLw@mail.gmail.com/

I am just wondering who is using FS_OPEN these days and whether
they would care about this change and if not, why are we doing it?

The argument that it is confusing to see FS_ACCESS/FS_MODIFY/FS_CLOSE
and not seeing FS_OPEN is only half true - it is common to see that
pattern when the file is already open when starting to watch, so application
should not break because of that pattern.

Thanks,
Amir.

Powered by blists - more mailing lists