From: Yann Droneaud <ydroneaud@opteya.com> To: "Pádraig Brady" <P@draigBrady.com> Cc: Andrew Morton <akpm@linux-foundation.org>, Heinrich Schuchardt <xypron.glpk@gmx.de>, Eric Paris <eparis@redhat.com>, Richard Guy Briggs <rgb@redhat.com>, Al Viro <viro@zeniv.linux.org.uk>, linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org, stable@vger.kernel.org, linux-api@vger.kernel.org, Jan Kara <jack@suse.cz>, Lino Sanfilippo <LinoSanfilippo@gmx.de>, Valdis Kletnieks <Valdis.Kletnieks@vt.edu>, Michael Kerrisk-manpages <mtk.manpages@gmail.com>, Yann Droneaud <ydroneaud@opteya.com> Subject: Re: [PATCH] fanotify: add a flag to allow setting O_CLOEXEC on event fd Date: Thu, 02 Oct 2014 16:44:17 +0200 [thread overview] Message-ID: <1412261057.5371.8.camel@localhost.localdomain> (raw) In-Reply-To: <542D1726.3040801@draigBrady.com> Hi, Le jeudi 02 octobre 2014 à 10:13 +0100, Pádraig Brady a écrit : > On 10/02/2014 08:52 AM, Yann Droneaud wrote: > > In order to not potentially break applications which were > > requesting O_CLOEXEC on event file descriptors but which > > actually need it to be not effective as the kernel currently > > ignore the flag, so the file descriptor is inherited accross > > exec regardless of O_CLOEXEC (please forgive me for the > > wording), this patch introduces FAN_FD_CLOEXEC flag to > > fanotify_init() so that application can request O_CLOEXEC > > to be effective. > > Newer application would use FAN_FD_CLOEXEC flag along > > O_CLOEXEC to enable close on exec on newly created > > file descriptor: > > > > fd = fanotify_init(FAN_CLOEXEC|FAN_NONBLOCK|FAN_FD_CLOEXEC, > > O_RDONLY|O_LARGEFILE|O_CLOEXEC|O_NOATIME); > > Ugh really? > IMHO there should be widespread or at least known breakage with > O_CLOEXEC before adding messiness like this. You should have read the other part of my message: > > While I believe fanotify_init() must enable close-on-exec > > when requested by userspace to prevent unwelcomed security > > issue, I understand your concerns regarding the possible > > breakage on userspace application requesting O_CLOEXEC > > but relying on it not being enable on file descriptor > > created for the events. > > > So with a new flag to fanotify_init(), we could allow > > newer applications to really enable O_CLOEXEC. > > > But I feel bad to have to force application to specify > > twice they want close on exec: > > - are you sure ? > > - are you really sure ? > > - is this your final answer ? > > ... I'm not really fond of this option. > It seems surprising to me that apps that would depend on > O_CLOEXEC being ineffective. > We have seen userspace developers making mistakes, and those mistakes were mistakenly ignored by the kernel until someone try to fix the mistake on kernel side, which broke the existing userspace application. > please reconsider this one. > I'm not going to promote this patch as it's a quick and dirty hack to demonstrate what would be the other option. Regards. -- Yann Droneaud OPTEYA
WARNING: multiple messages have this Message-ID (diff)
From: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org> To: "Pádraig Brady" <P@draigBrady.com> Cc: Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>, Heinrich Schuchardt <xypron.glpk-Mmb7MZpHnFY@public.gmane.org>, Eric Paris <eparis-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>, Richard Guy Briggs <rgb-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>, Al Viro <viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn@public.gmane.org>, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-fsdevel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Jan Kara <jack-AlSwsSmVLrQ@public.gmane.org>, Lino Sanfilippo <LinoSanfilippo-Mmb7MZpHnFY@public.gmane.org>, Valdis Kletnieks <Valdis.Kletnieks-PjAqaU27lzQ@public.gmane.org>, Michael Kerrisk-manpages <mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>, Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org> Subject: Re: [PATCH] fanotify: add a flag to allow setting O_CLOEXEC on event fd Date: Thu, 02 Oct 2014 16:44:17 +0200 [thread overview] Message-ID: <1412261057.5371.8.camel@localhost.localdomain> (raw) In-Reply-To: <542D1726.3040801-V8g9lnOeT5ydJdNcDFJN0w@public.gmane.org> Hi, Le jeudi 02 octobre 2014 à 10:13 +0100, Pádraig Brady a écrit : > On 10/02/2014 08:52 AM, Yann Droneaud wrote: > > In order to not potentially break applications which were > > requesting O_CLOEXEC on event file descriptors but which > > actually need it to be not effective as the kernel currently > > ignore the flag, so the file descriptor is inherited accross > > exec regardless of O_CLOEXEC (please forgive me for the > > wording), this patch introduces FAN_FD_CLOEXEC flag to > > fanotify_init() so that application can request O_CLOEXEC > > to be effective. > > Newer application would use FAN_FD_CLOEXEC flag along > > O_CLOEXEC to enable close on exec on newly created > > file descriptor: > > > > fd = fanotify_init(FAN_CLOEXEC|FAN_NONBLOCK|FAN_FD_CLOEXEC, > > O_RDONLY|O_LARGEFILE|O_CLOEXEC|O_NOATIME); > > Ugh really? > IMHO there should be widespread or at least known breakage with > O_CLOEXEC before adding messiness like this. You should have read the other part of my message: > > While I believe fanotify_init() must enable close-on-exec > > when requested by userspace to prevent unwelcomed security > > issue, I understand your concerns regarding the possible > > breakage on userspace application requesting O_CLOEXEC > > but relying on it not being enable on file descriptor > > created for the events. > > > So with a new flag to fanotify_init(), we could allow > > newer applications to really enable O_CLOEXEC. > > > But I feel bad to have to force application to specify > > twice they want close on exec: > > - are you sure ? > > - are you really sure ? > > - is this your final answer ? > > ... I'm not really fond of this option. > It seems surprising to me that apps that would depend on > O_CLOEXEC being ineffective. > We have seen userspace developers making mistakes, and those mistakes were mistakenly ignored by the kernel until someone try to fix the mistake on kernel side, which broke the existing userspace application. > please reconsider this one. > I'm not going to promote this patch as it's a quick and dirty hack to demonstrate what would be the other option. Regards. -- Yann Droneaud OPTEYA
next prev parent reply other threads:[~2014-10-02 14:44 UTC|newest] Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top 2014-09-24 13:11 [PATCHv8 0/6] Getting rid of get_unused_fd() / enable close-on-exec Yann Droneaud 2014-09-24 13:11 ` Yann Droneaud 2014-09-24 13:11 ` Yann Droneaud 2014-09-24 13:11 ` Yann Droneaud 2014-09-24 13:11 ` [PATCHv8 1/6] fanotify: enable close-on-exec on events' fd when requested in fanotify_init() Yann Droneaud 2014-09-25 20:57 ` Heinrich Schuchardt 2014-09-25 20:57 ` Heinrich Schuchardt 2014-09-26 8:58 ` Yann Droneaud 2014-09-27 7:26 ` Heinrich Schuchardt 2014-09-29 8:49 ` [PATCHv8.1] " Yann Droneaud 2014-09-29 11:50 ` Jan Kara 2014-10-01 22:36 ` Andrew Morton 2014-10-01 22:36 ` Andrew Morton 2014-10-02 6:20 ` Yann Droneaud 2014-10-02 6:50 ` Mihai Donțu 2014-10-02 7:52 ` [PATCH] fanotify: add a flag to allow setting O_CLOEXEC on event fd Yann Droneaud 2014-10-02 9:13 ` Pádraig Brady 2014-10-02 9:13 ` Pádraig Brady 2014-10-02 9:13 ` Pádraig Brady 2014-10-02 11:55 ` Michael Kerrisk (man-pages) 2014-10-02 14:44 ` Yann Droneaud [this message] 2014-10-02 14:44 ` Yann Droneaud 2014-10-02 10:44 ` [PATCHv8.1] fanotify: enable close-on-exec on events' fd when requested in fanotify_init() Jan Kara 2014-10-02 10:44 ` Jan Kara 2014-10-02 19:46 ` Andrew Morton 2014-10-02 19:46 ` Andrew Morton 2014-10-03 8:39 ` [PATCHv8.2] " Yann Droneaud 2014-10-03 8:39 ` Yann Droneaud 2014-09-24 13:11 ` [PATCHv8 2/6] ia64: trivial: replace get_unused_fd() by get_unused_fd_flags(0) Yann Droneaud 2014-09-24 13:11 ` Yann Droneaud 2014-09-24 13:11 ` [PATCHv8 3/6] ppc/cell: " Yann Droneaud 2014-09-24 13:11 ` Yann Droneaud 2014-09-24 13:11 ` [PATCHv8 4/6] binfmt_misc: " Yann Droneaud 2014-09-24 13:11 ` [PATCHv8 5/6] file: " Yann Droneaud 2014-09-24 13:11 ` [PATCHv8 6/6] file: remove get_unused_fd() macro Yann Droneaud
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=1412261057.5371.8.camel@localhost.localdomain \ --to=ydroneaud@opteya.com \ --cc=LinoSanfilippo@gmx.de \ --cc=P@draigBrady.com \ --cc=Valdis.Kletnieks@vt.edu \ --cc=akpm@linux-foundation.org \ --cc=eparis@redhat.com \ --cc=jack@suse.cz \ --cc=linux-api@vger.kernel.org \ --cc=linux-fsdevel@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=mtk.manpages@gmail.com \ --cc=rgb@redhat.com \ --cc=stable@vger.kernel.org \ --cc=viro@zeniv.linux.org.uk \ --cc=xypron.glpk@gmx.de \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.