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
On Tue, Sep 8, 2020 at 11:02 AM Xiaoming Ni <[email protected]> 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.
On Tue, Sep 8, 2020 at 3:53 PM Xiaoming Ni <[email protected]> wrote:
>
> On 2020/9/8 18:06, Amir Goldstein wrote:
> > On Tue, Sep 8, 2020 at 11:02 AM Xiaoming Ni <[email protected]> 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.
On 2020/9/8 18:06, Amir Goldstein wrote:
> On Tue, Sep 8, 2020 at 11:02 AM Xiaoming Ni <[email protected]> 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
On Tue, Sep 08, 2020 at 04:18:29PM +0300, Amir Goldstein wrote:
> On Tue, Sep 8, 2020 at 3:53 PM Xiaoming Ni <[email protected]> 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.
On Tue, Sep 8, 2020 at 8:19 PM Matthew Wilcox <[email protected]> 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 <[email protected]> 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.
On 2020/9/9 11:44, Amir Goldstein wrote:
> On Tue, Sep 8, 2020 at 8:19 PM Matthew Wilcox <[email protected]> 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 <[email protected]> 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
On Wed, Sep 9, 2020 at 10:00 AM Xiaoming Ni <[email protected]> wrote:
>
> On 2020/9/9 11:44, Amir Goldstein wrote:
> > On Tue, Sep 8, 2020 at 8:19 PM Matthew Wilcox <[email protected]> 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 <[email protected]> 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.
On Wed 09-09-20 10:36:57, Amir Goldstein wrote:
> On Wed, Sep 9, 2020 at 10:00 AM Xiaoming Ni <[email protected]> wrote:
> >
> > On 2020/9/9 11:44, Amir Goldstein wrote:
> > > On Tue, Sep 8, 2020 at 8:19 PM Matthew Wilcox <[email protected]> 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 <[email protected]> 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 <[email protected]>
SUSE Labs, CR
On Wed, Sep 9, 2020 at 2:11 PM Jan Kara <[email protected]> wrote:
>
> On Wed 09-09-20 10:36:57, Amir Goldstein wrote:
> > On Wed, Sep 9, 2020 at 10:00 AM Xiaoming Ni <[email protected]> wrote:
> > >
> > > On 2020/9/9 11:44, Amir Goldstein wrote:
> > > > On Tue, Sep 8, 2020 at 8:19 PM Matthew Wilcox <[email protected]> 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 <[email protected]> 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.
On Wed 09-09-20 19:03:07, Amir Goldstein wrote:
> On Wed, Sep 9, 2020 at 2:11 PM Jan Kara <[email protected]> wrote:
> >
> > On Wed 09-09-20 10:36:57, Amir Goldstein wrote:
> > > On Wed, Sep 9, 2020 at 10:00 AM Xiaoming Ni <[email protected]> wrote:
> > > >
> > > > On 2020/9/9 11:44, Amir Goldstein wrote:
> > > > > On Tue, Sep 8, 2020 at 8:19 PM Matthew Wilcox <[email protected]> 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 <[email protected]> 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 <[email protected]>
SUSE Labs, CR