Re: Question: Why is there no notification when a file is opened using filp_open()?
On Wed 09-09-20 19:03:07, Amir Goldstein wrote: > On Wed, Sep 9, 2020 at 2:11 PM Jan Kara wrote: > > > > On Wed 09-09-20 10:36:57, Amir Goldstein wrote: > > > On Wed, Sep 9, 2020 at 10:00 AM Xiaoming Ni wrote: > > > > > > > > On 2020/9/9 11:44, Amir Goldstein wrote: > > > > > On Tue, Sep 8, 2020 at 8:19 PM Matthew Wilcox > > > > > 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 > > > > >>> 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=msjki7kxq875me4h...@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
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 wrote: > > On Wed 09-09-20 10:36:57, Amir Goldstein wrote: > > On Wed, Sep 9, 2020 at 10:00 AM Xiaoming Ni wrote: > > > > > > On 2020/9/9 11:44, Amir Goldstein wrote: > > > > On Tue, Sep 8, 2020 at 8:19 PM Matthew Wilcox > > > > 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 > > > >>> 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=msjki7kxq875me4h...@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.
Re: Question: Why is there no notification when a file is opened using filp_open()?
On Wed 09-09-20 10:36:57, Amir Goldstein wrote: > On Wed, Sep 9, 2020 at 10:00 AM Xiaoming Ni wrote: > > > > On 2020/9/9 11:44, Amir Goldstein wrote: > > > On Tue, Sep 8, 2020 at 8:19 PM Matthew Wilcox 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 > > >>> 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 SUSE Labs, CR
Re: Question: Why is there no notification when a file is opened using filp_open()?
On Wed, Sep 9, 2020 at 10:00 AM Xiaoming Ni wrote: > > On 2020/9/9 11:44, Amir Goldstein wrote: > > On Tue, Sep 8, 2020 at 8:19 PM Matthew Wilcox 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 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.
Re: Question: Why is there no notification when a file is opened using filp_open()?
On 2020/9/9 11:44, Amir Goldstein wrote: On Tue, Sep 8, 2020 at 8:19 PM Matthew Wilcox 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 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
Re: Question: Why is there no notification when a file is opened using filp_open()?
On Tue, Sep 8, 2020 at 8:19 PM Matthew Wilcox 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 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.
Re: Question: Why is there no notification when a file is opened using filp_open()?
On Tue, Sep 08, 2020 at 04:18:29PM +0300, Amir Goldstein wrote: > On Tue, Sep 8, 2020 at 3:53 PM Xiaoming Ni 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.
Re: Question: Why is there no notification when a file is opened using filp_open()?
On 2020/9/8 18:06, Amir Goldstein wrote: On Tue, Sep 8, 2020 at 11:02 AM Xiaoming Ni 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
Re: Question: Why is there no notification when a file is opened using filp_open()?
On Tue, Sep 8, 2020 at 3:53 PM Xiaoming Ni wrote: > > On 2020/9/8 18:06, Amir Goldstein wrote: > > On Tue, Sep 8, 2020 at 11:02 AM Xiaoming Ni 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.
Re: Question: Why is there no notification when a file is opened using filp_open()?
On Tue, Sep 8, 2020 at 11:02 AM Xiaoming Ni 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.
Question: Why is there no notification when a file is opened using filp_open()?
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
