Re: [PATCHv8 1/6] fanotify: enable close-on-exec on events' fd when requested in fanotify_init()

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:

14 more quoted lines

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

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]:

55 more lines

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

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]:

36 more quoted lines

  The patch looks good. You can add:
Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

-- 
Jan Kara [off-list ref]
SUSE Labs, CR