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. Thanks, Amir.
