From: Heinrich Schuchardt <hidden> Date: 2014-09-25 20:57:23
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
Hi,
Le jeudi 25 septembre 2014 à 22:57 +0200, Heinrich Schuchardt a écrit :
On 24.09.2014 15:11, Yann Droneaud wrote:
quoted
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.
quoted
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.
From: Heinrich Schuchardt <hidden> Date: 2014-09-27 07:26:42
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 [off-list 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 :
quoted
On 24.09.2014 15:11, Yann Droneaud wrote:
quoted
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.
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#L1631https://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 <redacted>
Cc: Jan Kara <jack@suse.cz>
Cc: Valdis Kletnieks <redacted>
Cc: Michael Kerrisk-manpages <redacted>
Cc: Lino Sanfilippo <LinoSanfilippo@gmx.de>
Cc: Richard Guy Briggs <redacted>
Cc: Eric Paris <eparis@redhat.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: stable@vger.kernel.org
Cc: linux-api@vger.kernel.org
Reviewed by: Heinrich Schuchardt [off-list ref]
Signed-off-by: Yann Droneaud <redacted>
---
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(-)
From: Jan Kara <jack@suse.cz> Date: 2014-09-29 11:50:27
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#L1631https://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 <redacted>
Cc: Jan Kara <jack@suse.cz>
Cc: Valdis Kletnieks <redacted>
Cc: Michael Kerrisk-manpages <redacted>
Cc: Lino Sanfilippo <LinoSanfilippo@gmx.de>
Cc: Richard Guy Briggs <redacted>
Cc: Eric Paris <eparis@redhat.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: stable@vger.kernel.org
Cc: linux-api@vger.kernel.org
Reviewed by: Heinrich Schuchardt [off-list ref]
Signed-off-by: Yann Droneaud <redacted>
The patch looks good. You can add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
quoted hunk
---
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(-)