Linux-Fsdevel Archive on lore.kernel.org
 help / color / Atom feed
From: Jan Kara <jack@suse.cz>
To: Matthew Bobrowski <repnop@google.com>
Cc: Christian Brauner <christian.brauner@ubuntu.com>,
	Jan Kara <jack@suse.cz>,
	amir73il@gmail.com, linux-api@vger.kernel.org,
	linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH 2/2] fanotify: Add pidfd support to the fanotify API
Date: Wed, 21 Apr 2021 10:04:49 +0200
Message-ID: <20210421080449.GK8706@quack2.suse.cz> (raw)
In-Reply-To: <YH4+Swki++PHIwpY@google.com>

On Tue 20-04-21 12:36:59, Matthew Bobrowski wrote:
> On Mon, Apr 19, 2021 at 05:02:33PM +0200, Christian Brauner wrote:
> > A general question about struct fanotify_event_metadata and its
> > extensibility model:
> > looking through the code it seems that this struct is read via
> > fanotify_rad(). So the user is expected to supply a buffer with at least
> > 
> > #define FAN_EVENT_METADATA_LEN (sizeof(struct fanotify_event_metadata))
> > 
> > bytes. In addition you can return the info to the user about how many
> > bytes the kernel has written from fanotify_read().
> > 
> > So afaict extending fanotify_event_metadata should be _fairly_
> > straightforward, right? It would essentially the complement to
> > copy_struct_from_user() which Aleksa and I added (1 or 2 years ago)
> > which deals with user->kernel and you're dealing with kernel->user:
> > - If the user supplied a buffer smaller than the minimum known struct
> >   size -> reject.
> > - If the user supplied a buffer < smaller than what the current kernel
> >   supports -> copy only what userspace knows about, and return the size
> >   userspace knows about.
> > - If the user supplied a buffer that is larger than what the current
> >   kernel knows about -> copy only what the kernel knows about, zero the
> >   rest, and return the kernel size.
> > 
> > Extension should then be fairly straightforward (64bit aligned
> > increments)?
> 
> You'd think that it's fairly straightforward, but I have a feeling
> that the whole fanotify_event_metadata extensibility discussion and
> the current limitation to do so revolves around whether it can be
> achieved in a way which can guarantee that no userspace applications
> would break. I think the answer to this is that there's no guarantee
> because of <<reasons>>, so the decision to extend fanotify's feature
> set was done via other means i.e. introduction of additional
> structures.

There's no real problem extending fanotify_event_metadata. We already have
multiple extended version of that structure in use (see e.g. FAN_REPORT_FID
flag and its effect, extended versions of the structure in
include/uapi/linux/fanotify.h). The key for backward compatibility is to
create extended struct only when explicitely requested by a flag when
creating notification group - and that would be the case here -
FAN_REPORT_PIDFD or how you called it. It is just that extending the
structure means adding 8 bytes to each event and parsing extended structure
is more cumbersome than just fetching s32 from a well known location.

On the other hand extended structure is self-describing (i.e., you can tell
the meaning of all the fields just from the event you receive) while
reusing 'pid' field means that you have to know how the notification group
was created (whether FAN_REPORT_PIDFD was used or not) to be able to
interpret the contents of the event. Actually I think the self-describing
feature of fanotify event stream is useful (e.g. when application manages
multiple fanotify groups or when fanotify group descriptors are passed
among processes) so now I'm more leaning towards using the extended
structure instead of reusing 'pid' as Christian suggests. I'm sorry for the
confusion.

								Honza

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

  reply index

Thread overview: 35+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-15 23:21 [PATCH 0/2] fanotify: Adding " Matthew Bobrowski
2021-04-15 23:22 ` [PATCH 1/2] pidfd_create(): remove static qualifier and declare pidfd_create() in linux/pid.h Matthew Bobrowski
2021-04-19 10:13   ` Jan Kara
2021-04-19 12:50   ` Christian Brauner
2021-04-20  0:17     ` Matthew Bobrowski
2021-04-15 23:22 ` [PATCH 2/2] fanotify: Add pidfd support to the fanotify API Matthew Bobrowski
2021-04-16  6:27   ` Amir Goldstein
2021-04-16  7:05     ` Matthew Bobrowski
2021-04-16  7:53       ` Amir Goldstein
2021-04-16  8:08         ` Matthew Bobrowski
2021-04-19 13:02     ` Christian Brauner
2021-04-19 10:21   ` Jan Kara
2021-04-20  1:35     ` Matthew Bobrowski
2021-04-19 13:20   ` Christian Brauner
2021-04-19 13:53     ` Amir Goldstein
2021-04-19 14:44       ` Christian Brauner
2021-04-19 13:55     ` Jan Kara
2021-04-19 15:02       ` Christian Brauner
2021-04-20  2:36         ` Matthew Bobrowski
2021-04-21  8:04           ` Jan Kara [this message]
2021-04-21  9:29             ` Amir Goldstein
2021-04-21 10:00               ` Jan Kara
2021-04-21 10:12                 ` Amir Goldstein
2021-04-21 13:48                   ` Jan Kara
2021-04-21 14:46                     ` Christian Brauner
2021-04-22 23:06             ` Matthew Bobrowski
2021-04-23  7:39               ` Amir Goldstein
2021-04-23  8:02                 ` Matthew Bobrowski
2021-04-23  8:14                   ` Amir Goldstein
2021-04-26 10:26                     ` Matthew Bobrowski
2021-04-26 11:11                       ` Amir Goldstein
2021-04-27  3:35                         ` Matthew Bobrowski
2021-04-27  5:14                           ` Amir Goldstein
2021-04-28 22:53                             ` Matthew Bobrowski
2021-04-19 12:34 ` [PATCH 0/2] fanotify: Adding " Christian Brauner

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=20210421080449.GK8706@quack2.suse.cz \
    --to=jack@suse.cz \
    --cc=amir73il@gmail.com \
    --cc=christian.brauner@ubuntu.com \
    --cc=linux-api@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=repnop@google.com \
    /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: link

Linux-Fsdevel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-fsdevel/0 linux-fsdevel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-fsdevel linux-fsdevel/ https://lore.kernel.org/linux-fsdevel \
		linux-fsdevel@vger.kernel.org
	public-inbox-index linux-fsdevel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-fsdevel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git