# Re: \[PATCHv8 1/6\] fanotify: enable close-on-exec on events' fd when requested in fanotify\_init() - List: linux-api - 5 messages, 4 authors - Page: https://ratatoskr.run/linux-api/2014/09/88388/t ## Re: \[PATCHv8 1/6\] fanotify: enable close-on-exec on events' fd when requested in fanotify\_init() - From: Heinrich Schuchardt \ - Date: 2014-09-25 20:57:23 - Message: https://ratatoskr.run/linux-api/2014/09/88388.md ```text On 24.09.2014 15:11, Yann Droneaud wrote: > According to commit 80af258867648 ('fanotify: groups can specify > their f_flags for new fd'), file descriptors created as part of > file access notification events inherit flags from the > event_f_flags argument passed to syscall fanotify_init(2). > > So while it is legal for userspace to call fanotify_init() with > O_CLOEXEC as part of its second argument, O_CLOEXEC is currently > silently ignored. > > Indeed event_f_flags are only given to dentry_open(), which only > seems to care about O_ACCMODE and O_PATH in do_dentry_open(), > O_DIRECT in open_check_o_direct() and O_LARGEFILE in > generic_file_open(). I tested on kernel 3.17.0-rc5. I passed O_CLOEXEC in event_f_flags. When I called fcnt(event_metadata->fd, F_GETFD) it did not return FD_CLOEXEC. So I can confirm your observation that O_CLOEXEC is not working as expected. I found this definition #define get_unused_fd() get_unused_fd_flags(0) So essentially when get_unused_fd() is called for a fanotify event O_CLOEXEC is ignored. This is what your patch fixes. > > More, there's no effective check on event_f_flags value that > would catch unknown / unsupported values, unlike the one on > f_flags argument of the syscall (see FAN_ALL_INIT_FLAGS in > include/uapi/linux/fanotify.h). The fanotify_init(2) man page describes which flags are allowable in event_f_flags. Could you, please, explain why the following code in fanotify_user.c is not to be considered an effective check: if (event_f_flags & ~FANOTIFY_INIT_ALL_EVENT_F_BITS) return -EINVAL; switch (event_f_flags & O_ACCMODE) { case O_RDONLY: case O_RDWR: case O_WRONLY: break; default: return -EINVAL; } I CC Jan Kara as he reviewed the code. Best regards Heinrich Schuchardt ``` ## Re: \[PATCHv8 1/6\] fanotify: enable close-on-exec on events' fd when requested in fanotify\_init() - From: Yann Droneaud \ - Date: 2014-09-26 08:58:18 - Message: https://ratatoskr.run/linux-api/2014/09/88400.md ```text Hi, Le jeudi 25 septembre 2014 à 22:57 +0200, Heinrich Schuchardt a écrit : > On 24.09.2014 15:11, Yann Droneaud wrote: > > According to commit 80af258867648 ('fanotify: groups can specify > > their f_flags for new fd'), file descriptors created as part of > > file access notification events inherit flags from the > > event_f_flags argument passed to syscall fanotify_init(2). > > > > So while it is legal for userspace to call fanotify_init() with > > O_CLOEXEC as part of its second argument, O_CLOEXEC is currently > > silently ignored. > > > > Indeed event_f_flags are only given to dentry_open(), which only > > seems to care about O_ACCMODE and O_PATH in do_dentry_open(), > > O_DIRECT in open_check_o_direct() and O_LARGEFILE in > > generic_file_open(). > > I tested on kernel 3.17.0-rc5. I passed O_CLOEXEC in event_f_flags. > When I called fcnt(event_metadata->fd, F_GETFD) it did not return > FD_CLOEXEC. So I can confirm your observation that O_CLOEXEC is not > working as expected. > > I found this definition > #define get_unused_fd() get_unused_fd_flags(0) > > So essentially when get_unused_fd() is called for a fanotify event > O_CLOEXEC is ignored. > > This is what your patch fixes. > > > > > More, there's no effective check on event_f_flags value that > > would catch unknown / unsupported values, unlike the one on > > f_flags argument of the syscall (see FAN_ALL_INIT_FLAGS in > > include/uapi/linux/fanotify.h). > > The fanotify_init(2) man page describes which flags are allowable in > event_f_flags. > > Could you, please, explain why the following code in fanotify_user.c is > not to be considered an effective check: > > if (event_f_flags & ~FANOTIFY_INIT_ALL_EVENT_F_BITS) > return -EINVAL; > > switch (event_f_flags & O_ACCMODE) { > case O_RDONLY: > case O_RDWR: > case O_WRONLY: > break; > default: > return -EINVAL; > } > > I CC Jan Kara as he reviewed the code. > I missed the opportunity to update my commit message. I've sent my initial version of the patch (with the same wording) on 5th, January, 2014: http://mid.gmane.org/3d9591f81e62a78a726721c8052b3910870db35e.1388952061.git.ydroneaud@opteya.com http://lkml.kernel.org/r/3d9591f81e62a78a726721c8052b3910870db35e.1388952061.git.ydroneaud@opteya.com The patch was sent again on March, 11: http://lkml.kernel.org/r/baab31b572b216d13f2149bdf07e0f79a1bae660.1394532336.git.ydroneaud@opteya.com http://mid.gmane.org/baab31b572b216d13f2149bdf07e0f79a1bae660.1394532336.git.ydroneaud@opteya.com And another time, on June, 1st: http://lkml.kernel.org/r/2c6ab28980f0007ea3b9afa7ecd7497806a6a451.1401630396.git.ydroneaud@opteya.com http://mid.gmane.org/2c6ab28980f0007ea3b9afa7ecd7497806a6a451.1401630396.git.ydroneaud@opteya.com So as you can see, my patch predate yours: http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=48149e9d3a7e924010a0daab30a6197b7d7b6580 But I have to apologize: I haven't noticed your patch was merged between my previous submission and the current one. My bad. I will update the commit message to remove my obsolete comment on the input parameter check. Thanks again for review and testing. Regards. -- Yann Droneaud OPTEYA ``` ## Re: \[PATCHv8 1/6\] fanotify: enable close-on-exec on events' fd when requested in fanotify\_init() - From: Heinrich Schuchardt \ - Date: 2014-09-27 07:26:42 - Message: https://ratatoskr.run/linux-api/2014/09/88431.md ```text Hello Andrew, please, add the patch described in https://lkml.org/lkml/2014/9/24/967 to the MM tree. I have tested kernel 3.17.0-rc6 with and without the patch and it fixes the described error. As the patch is valid independent of the rest of the patch set, there is no reason to wait for the rest to be merged. You may add Reviewed by: Heinrich Schuchardt [ref] Best regards Heinrich Schuchardt On 26.09.2014 10:58, Yann Droneaud wrote: > Hi, > > Le jeudi 25 septembre 2014 à 22:57 +0200, Heinrich Schuchardt a écrit : >> On 24.09.2014 15:11, Yann Droneaud wrote: >>> According to commit 80af258867648 ('fanotify: groups can specify >>> their f_flags for new fd'), file descriptors created as part of >>> file access notification events inherit flags from the >>> event_f_flags argument passed to syscall fanotify_init(2). >>> >>> So while it is legal for userspace to call fanotify_init() with >>> O_CLOEXEC as part of its second argument, O_CLOEXEC is currently >>> silently ignored. >>> >>> Indeed event_f_flags are only given to dentry_open(), which only >>> seems to care about O_ACCMODE and O_PATH in do_dentry_open(), >>> O_DIRECT in open_check_o_direct() and O_LARGEFILE in >>> generic_file_open(). >> >> I tested on kernel 3.17.0-rc5. I passed O_CLOEXEC in event_f_flags. >> When I called fcnt(event_metadata->fd, F_GETFD) it did not return >> FD_CLOEXEC. So I can confirm your observation that O_CLOEXEC is not >> working as expected. >> >> I found this definition >> #define get_unused_fd() get_unused_fd_flags(0) >> >> So essentially when get_unused_fd() is called for a fanotify event >> O_CLOEXEC is ignored. >> >> This is what your patch fixes. >> ``` ## \[PATCHv8.1\] fanotify: enable close-on-exec on events' fd when requested in fanotify\_init() - From: Yann Droneaud \ - Date: 2014-09-29 08:49:15 - Message: https://ratatoskr.run/linux-api/2014/09/88452.md ```text According to commit 80af258867648 ('fanotify: groups can specify their f_flags for new fd'), file descriptors created as part of file access notification events inherit flags from the event_f_flags argument passed to syscall fanotify_init(2). So while it is legal for userspace to call fanotify_init() with O_CLOEXEC as part of its second argument, O_CLOEXEC is currently silently ignored. Indeed event_f_flags are only given to dentry_open(), which only seems to care about O_ACCMODE and O_PATH in do_dentry_open(), O_DIRECT in open_check_o_direct() and O_LARGEFILE in generic_file_open(). But it seems logical to set close-on-exec flag on the file descriptor if userspace is allowed to request it with O_CLOEXEC. In fact, according to some lookup on http://codesearch.debian.net/ and various search engine, there's already some userspace code requesting it: - in systemd's readahead[2]: fanotify_fd = fanotify_init(FAN_CLOEXEC|FAN_NONBLOCK, O_RDONLY|O_LARGEFILE|O_CLOEXEC|O_NOATIME); - in clsync[3]: #define FANOTIFY_EVFLAGS (O_LARGEFILE|O_RDONLY|O_CLOEXEC) int fanotify_d = fanotify_init(FANOTIFY_FLAGS, FANOTIFY_EVFLAGS); - in examples [4] from "Filesystem monitoring in the Linux kernel" article[5] by Aleksander Morgado: if ((fanotify_fd = fanotify_init (FAN_CLOEXEC, O_RDONLY | O_CLOEXEC | O_LARGEFILE)) < 0) Lookup also returned some wrong usage of the syscall: - in Gonk HAL from Mozilla Firefox OS sources[6]: mFd = fanotify_init(FAN_CLASS_NOTIF, FAN_CLOEXEC); Adding support for O_CLOEXEC in fanotify_init() won't magically enable it for Gonk since FAN_CLOEXEC is defined as 0x1, which is likely equal to O_WRONLY when used in open flag context. In the other hand, it won't hurt it either. So this patch replaces call to macro get_unused_fd() by a call to function get_unused_fd_flags() with event_f_flags value as argument. This way O_CLOEXEC flag in the second argument of fanotify_init(2) syscall is interpreted so that close-on-exec get enabled when requested. [1] http://blog.ffwll.ch/2013/11/botching-up-ioctls.html [2] http://cgit.freedesktop.org/systemd/systemd/tree/src/readahead/readahead-collect.c?id=v208#n294 [3] https://github.com/xaionaro/clsync/blob/v0.2.1/sync.c#L1631 https://github.com/xaionaro/clsync/blob/v0.2.1/configuration.h#L38 [4] http://www.lanedo.com/~aleksander/fanotify/fanotify-example.c [5] http://www.lanedo.com/2013/filesystem-monitoring-linux-kernel/ [6] http://hg.mozilla.org/mozilla-central/file/325c74addeba/hal/gonk/GonkDiskSpaceWatcher.cpp#l167 Link: http://lkml.kernel.org/r/cover.1394532336.git.ydroneaud@opteya.com Cc: Heinrich Schuchardt Cc: Jan Kara Cc: Valdis Kletnieks Cc: Michael Kerrisk-manpages Cc: Lino Sanfilippo Cc: Richard Guy Briggs Cc: Eric Paris Cc: Al Viro Cc: Andrew Morton Cc: Cc: Reviewed by: Heinrich Schuchardt [ref] Signed-off-by: Yann Droneaud --- Hi Andrew and Henrich, Please find an updated patch with a commit message fixed regarding the obsolote comments on code which is now updated, thanks to Heinrich's patch. Changes from v8: - fixed commit message - added Reviewed-by: Regards. Yann Droneaud, OPTEYA fs/notify/fanotify/fanotify_user.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c index b13992a41bd9..c991616acca9 100644 --- a/fs/notify/fanotify/fanotify_user.c +++ b/fs/notify/fanotify/fanotify_user.c @@ -78,7 +78,7 @@ static int create_fd(struct fsnotify_group *group, pr_debug("%s: group=%p event=%p\n", __func__, group, event); - client_fd = get_unused_fd(); + client_fd = get_unused_fd_flags(group->fanotify_data.f_flags); if (client_fd < 0) return client_fd; -- 1.9.3 ``` ## Re: \[PATCHv8.1\] fanotify: enable close-on-exec on events' fd when requested in fanotify\_init() - From: Jan Kara \ - Date: 2014-09-29 11:50:27 - Message: https://ratatoskr.run/linux-api/2014/09/88455.md ```text On Mon 29-09-14 10:49:15, Yann Droneaud wrote: > According to commit 80af258867648 ('fanotify: groups can specify > their f_flags for new fd'), file descriptors created as part of > file access notification events inherit flags from the > event_f_flags argument passed to syscall fanotify_init(2). > > So while it is legal for userspace to call fanotify_init() with > O_CLOEXEC as part of its second argument, O_CLOEXEC is currently > silently ignored. > > Indeed event_f_flags are only given to dentry_open(), which only > seems to care about O_ACCMODE and O_PATH in do_dentry_open(), > O_DIRECT in open_check_o_direct() and O_LARGEFILE in > generic_file_open(). > > But it seems logical to set close-on-exec flag on the file > descriptor if userspace is allowed to request it with O_CLOEXEC. > > In fact, according to some lookup on http://codesearch.debian.net/ > and various search engine, there's already some userspace code > requesting it: > > - in systemd's readahead[2]: > > fanotify_fd = fanotify_init(FAN_CLOEXEC|FAN_NONBLOCK, O_RDONLY|O_LARGEFILE|O_CLOEXEC|O_NOATIME); > > - in clsync[3]: > > #define FANOTIFY_EVFLAGS (O_LARGEFILE|O_RDONLY|O_CLOEXEC) > > int fanotify_d = fanotify_init(FANOTIFY_FLAGS, FANOTIFY_EVFLAGS); > > - in examples [4] from "Filesystem monitoring in the Linux > kernel" article[5] by Aleksander Morgado: > > if ((fanotify_fd = fanotify_init (FAN_CLOEXEC, > O_RDONLY | O_CLOEXEC | O_LARGEFILE)) < 0) > > Lookup also returned some wrong usage of the syscall: > > - in Gonk HAL from Mozilla Firefox OS sources[6]: > > mFd = fanotify_init(FAN_CLASS_NOTIF, FAN_CLOEXEC); > > Adding support for O_CLOEXEC in fanotify_init() won't magically > enable it for Gonk since FAN_CLOEXEC is defined as 0x1, which > is likely equal to O_WRONLY when used in open flag context. In > the other hand, it won't hurt it either. > > So this patch replaces call to macro get_unused_fd() by a call > to function get_unused_fd_flags() with event_f_flags value as > argument. This way O_CLOEXEC flag in the second argument of > fanotify_init(2) syscall is interpreted so that close-on-exec > get enabled when requested. > > [1] http://blog.ffwll.ch/2013/11/botching-up-ioctls.html > [2] http://cgit.freedesktop.org/systemd/systemd/tree/src/readahead/readahead-collect.c?id=v208#n294 > [3] https://github.com/xaionaro/clsync/blob/v0.2.1/sync.c#L1631 > https://github.com/xaionaro/clsync/blob/v0.2.1/configuration.h#L38 > [4] http://www.lanedo.com/~aleksander/fanotify/fanotify-example.c > [5] http://www.lanedo.com/2013/filesystem-monitoring-linux-kernel/ > [6] http://hg.mozilla.org/mozilla-central/file/325c74addeba/hal/gonk/GonkDiskSpaceWatcher.cpp#l167 > > Link: http://lkml.kernel.org/r/cover.1394532336.git.ydroneaud@opteya.com > Cc: Heinrich Schuchardt > Cc: Jan Kara > Cc: Valdis Kletnieks > Cc: Michael Kerrisk-manpages > Cc: Lino Sanfilippo > Cc: Richard Guy Briggs > Cc: Eric Paris > Cc: Al Viro > Cc: Andrew Morton > Cc: > Cc: > Reviewed by: Heinrich Schuchardt [ref] > Signed-off-by: Yann Droneaud The patch looks good. You can add: Reviewed-by: Jan Kara Honza > --- > Hi Andrew and Henrich, > > Please find an updated patch with a commit message fixed regarding > the obsolote comments on code which is now updated, thanks to > Heinrich's patch. > > Changes from v8: > - fixed commit message > - added Reviewed-by: > > Regards. > > Yann Droneaud, > OPTEYA > > fs/notify/fanotify/fanotify_user.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c > index b13992a41bd9..c991616acca9 100644 > --- a/fs/notify/fanotify/fanotify_user.c > +++ b/fs/notify/fanotify/fanotify_user.c > @@ -78,7 +78,7 @@ static int create_fd(struct fsnotify_group *group, > > pr_debug("%s: group=%p event=%p\n", __func__, group, event); > > - client_fd = get_unused_fd(); > + client_fd = get_unused_fd_flags(group->fanotify_data.f_flags); > if (client_fd < 0) > return client_fd; > > -- > 1.9.3 > -- Jan Kara [ref] SUSE Labs, CR ```