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