All of lore.kernel.org
 help / color / mirror / Atom feed
* Question: Why is there no notification when a file is opened using filp_open()?
@ 2020-09-08  8:01 Xiaoming Ni
  2020-09-08 10:06 ` Amir Goldstein
  0 siblings, 1 reply; 11+ messages in thread
From: Xiaoming Ni @ 2020-09-08  8:01 UTC (permalink / raw)
  To: jack, amir73il; +Cc: linux-fsdevel, Linux Kernel Mailing List, wangle6

The file opening action on the system may be from user-mode sys_open() 
or kernel-mode filp_open().
Currently, fsnotify_open() is invoked in do_sys_openat2().
But filp_open() is not notified. Why? Is this an omission?

Do we need to call fsnotify_open() in filp_open() or  do_filp_open() to 
ensure that both user-mode and kernel-mode file opening operations can 
be notified?

Thanks
Xiaoming Ni

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Question: Why is there no notification when a file is opened using filp_open()?
  2020-09-08  8:01 Question: Why is there no notification when a file is opened using filp_open()? Xiaoming Ni
@ 2020-09-08 10:06 ` Amir Goldstein
  2020-09-08 12:53   ` Xiaoming Ni
  0 siblings, 1 reply; 11+ messages in thread
From: Amir Goldstein @ 2020-09-08 10:06 UTC (permalink / raw)
  To: Xiaoming Ni; +Cc: Jan Kara, linux-fsdevel, Linux Kernel Mailing List, wangle6

On Tue, Sep 8, 2020 at 11:02 AM Xiaoming Ni <nixiaoming@huawei.com> wrote:
>
> The file opening action on the system may be from user-mode sys_open()
> or kernel-mode filp_open().
> Currently, fsnotify_open() is invoked in do_sys_openat2().
> But filp_open() is not notified. Why? Is this an omission?
>
> Do we need to call fsnotify_open() in filp_open() or  do_filp_open() to
> ensure that both user-mode and kernel-mode file opening operations can
> be notified?
>

Do you have a specific use case of kernel filp_open() in mind?

Thanks,
Amir.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Question: Why is there no notification when a file is opened using filp_open()?
  2020-09-08 10:06 ` Amir Goldstein
@ 2020-09-08 12:53   ` Xiaoming Ni
  2020-09-08 13:18     ` Amir Goldstein
  0 siblings, 1 reply; 11+ messages in thread
From: Xiaoming Ni @ 2020-09-08 12:53 UTC (permalink / raw)
  To: Amir Goldstein
  Cc: Jan Kara, linux-fsdevel, Linux Kernel Mailing List, wangle6

On 2020/9/8 18:06, Amir Goldstein wrote:
> On Tue, Sep 8, 2020 at 11:02 AM Xiaoming Ni <nixiaoming@huawei.com> wrote:
>>
>> The file opening action on the system may be from user-mode sys_open()
>> or kernel-mode filp_open().
>> Currently, fsnotify_open() is invoked in do_sys_openat2().
>> But filp_open() is not notified. Why? Is this an omission?
>>
>> Do we need to call fsnotify_open() in filp_open() or  do_filp_open() to
>> ensure that both user-mode and kernel-mode file opening operations can
>> be notified?
>>
> 
> Do you have a specific use case of kernel filp_open() in mind?
> 

For example, in fs/coredump.c, do_coredump() calls filp_open() to 
generate core files.
In this scenario, the fsnotify_open() notification is missing.

Thanks
Xiaoming Ni


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Question: Why is there no notification when a file is opened using filp_open()?
  2020-09-08 12:53   ` Xiaoming Ni
@ 2020-09-08 13:18     ` Amir Goldstein
  2020-09-08 17:18       ` Matthew Wilcox
  0 siblings, 1 reply; 11+ messages in thread
From: Amir Goldstein @ 2020-09-08 13:18 UTC (permalink / raw)
  To: Xiaoming Ni; +Cc: Jan Kara, linux-fsdevel, Linux Kernel Mailing List, wangle6

On Tue, Sep 8, 2020 at 3:53 PM Xiaoming Ni <nixiaoming@huawei.com> wrote:
>
> On 2020/9/8 18:06, Amir Goldstein wrote:
> > On Tue, Sep 8, 2020 at 11:02 AM Xiaoming Ni <nixiaoming@huawei.com> wrote:
> >>
> >> The file opening action on the system may be from user-mode sys_open()
> >> or kernel-mode filp_open().
> >> Currently, fsnotify_open() is invoked in do_sys_openat2().
> >> But filp_open() is not notified. Why? Is this an omission?
> >>
> >> Do we need to call fsnotify_open() in filp_open() or  do_filp_open() to
> >> ensure that both user-mode and kernel-mode file opening operations can
> >> be notified?
> >>
> >
> > Do you have a specific use case of kernel filp_open() in mind?
> >
>
> 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.

Thanks,
Amir.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Question: Why is there no notification when a file is opened using filp_open()?
  2020-09-08 13:18     ` Amir Goldstein
@ 2020-09-08 17:18       ` Matthew Wilcox
  2020-09-09  3:44         ` Amir Goldstein
  0 siblings, 1 reply; 11+ messages in thread
From: Matthew Wilcox @ 2020-09-08 17:18 UTC (permalink / raw)
  To: Amir Goldstein
  Cc: Xiaoming Ni, Jan Kara, linux-fsdevel, Linux Kernel Mailing List, wangle6

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@huawei.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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Question: Why is there no notification when a file is opened using filp_open()?
  2020-09-08 17:18       ` Matthew Wilcox
@ 2020-09-09  3:44         ` Amir Goldstein
  2020-09-09  6:59           ` Xiaoming Ni
  0 siblings, 1 reply; 11+ messages in thread
From: Amir Goldstein @ 2020-09-09  3:44 UTC (permalink / raw)
  To: Matthew Wilcox
  Cc: Xiaoming Ni, Jan Kara, linux-fsdevel, Linux Kernel Mailing List, wangle6

On Tue, Sep 8, 2020 at 8:19 PM Matthew Wilcox <willy@infradead.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@huawei.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.

Thanks,
Amir.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Question: Why is there no notification when a file is opened using filp_open()?
  2020-09-09  3:44         ` Amir Goldstein
@ 2020-09-09  6:59           ` Xiaoming Ni
  2020-09-09  7:36             ` Amir Goldstein
  0 siblings, 1 reply; 11+ messages in thread
From: Xiaoming Ni @ 2020-09-09  6:59 UTC (permalink / raw)
  To: Amir Goldstein, Matthew Wilcox
  Cc: Jan Kara, linux-fsdevel, Linux Kernel Mailing List, wangle6

On 2020/9/9 11:44, Amir Goldstein wrote:
> On Tue, Sep 8, 2020 at 8:19 PM Matthew Wilcox <willy@infradead.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@huawei.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?

Thanks
Xiaoming Ni

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Question: Why is there no notification when a file is opened using filp_open()?
  2020-09-09  6:59           ` Xiaoming Ni
@ 2020-09-09  7:36             ` Amir Goldstein
  2020-09-09 11:11               ` Jan Kara
  0 siblings, 1 reply; 11+ messages in thread
From: Amir Goldstein @ 2020-09-09  7:36 UTC (permalink / raw)
  To: Xiaoming Ni
  Cc: Matthew Wilcox, Jan Kara, linux-fsdevel,
	Linux Kernel Mailing List, wangle6

On Wed, Sep 9, 2020 at 10:00 AM Xiaoming Ni <nixiaoming@huawei.com> wrote:
>
> On 2020/9/9 11:44, Amir Goldstein wrote:
> > On Tue, Sep 8, 2020 at 8:19 PM Matthew Wilcox <willy@infradead.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@huawei.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.

Thanks,
Amir.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Question: Why is there no notification when a file is opened using filp_open()?
  2020-09-09  7:36             ` Amir Goldstein
@ 2020-09-09 11:11               ` Jan Kara
  2020-09-09 16:03                 ` Amir Goldstein
  0 siblings, 1 reply; 11+ messages in thread
From: Jan Kara @ 2020-09-09 11:11 UTC (permalink / raw)
  To: Amir Goldstein
  Cc: Xiaoming Ni, Matthew Wilcox, Jan Kara, linux-fsdevel,
	Linux Kernel Mailing List, wangle6

On Wed 09-09-20 10:36:57, Amir Goldstein wrote:
> On Wed, Sep 9, 2020 at 10:00 AM Xiaoming Ni <nixiaoming@huawei.com> wrote:
> >
> > On 2020/9/9 11:44, Amir Goldstein wrote:
> > > On Tue, Sep 8, 2020 at 8:19 PM Matthew Wilcox <willy@infradead.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@huawei.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
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.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Question: Why is there no notification when a file is opened using filp_open()?
  2020-09-09 11:11               ` Jan Kara
@ 2020-09-09 16:03                 ` Amir Goldstein
  2020-09-10  8:14                   ` Jan Kara
  0 siblings, 1 reply; 11+ messages in thread
From: Amir Goldstein @ 2020-09-09 16:03 UTC (permalink / raw)
  To: Jan Kara
  Cc: Xiaoming Ni, Matthew Wilcox, linux-fsdevel,
	Linux Kernel Mailing List, wangle6

On Wed, Sep 9, 2020 at 2:11 PM Jan Kara <jack@suse.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@huawei.com> wrote:
> > >
> > > On 2020/9/9 11:44, Amir Goldstein wrote:
> > > > On Tue, Sep 8, 2020 at 8:19 PM Matthew Wilcox <willy@infradead.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@huawei.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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Question: Why is there no notification when a file is opened using filp_open()?
  2020-09-09 16:03                 ` Amir Goldstein
@ 2020-09-10  8:14                   ` Jan Kara
  0 siblings, 0 replies; 11+ messages in thread
From: Jan Kara @ 2020-09-10  8:14 UTC (permalink / raw)
  To: Amir Goldstein
  Cc: Jan Kara, Xiaoming Ni, Matthew Wilcox, linux-fsdevel,
	Linux Kernel Mailing List, wangle6

On Wed 09-09-20 19:03:07, Amir Goldstein wrote:
> On Wed, Sep 9, 2020 at 2:11 PM Jan Kara <jack@suse.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@huawei.com> wrote:
> > > >
> > > > On 2020/9/9 11:44, Amir Goldstein wrote:
> > > > > On Tue, Sep 8, 2020 at 8:19 PM Matthew Wilcox <willy@infradead.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@huawei.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.

Yes.

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

Right, I remember that one :)

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

I'd be interested who is using FS_OPEN these days as well. And you're right
that without users the discussion is kind of moot.

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

Good point.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2020-09-10  8:26 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-08  8:01 Question: Why is there no notification when a file is opened using filp_open()? Xiaoming Ni
2020-09-08 10:06 ` Amir Goldstein
2020-09-08 12:53   ` Xiaoming Ni
2020-09-08 13:18     ` Amir Goldstein
2020-09-08 17:18       ` Matthew Wilcox
2020-09-09  3:44         ` Amir Goldstein
2020-09-09  6:59           ` Xiaoming Ni
2020-09-09  7:36             ` Amir Goldstein
2020-09-09 11:11               ` Jan Kara
2020-09-09 16:03                 ` Amir Goldstein
2020-09-10  8:14                   ` Jan Kara

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.