From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-13.3 required=3.0 tests=BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_IN_DEF_DKIM_WL autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 8B47BC433ED for ; Mon, 26 Apr 2021 10:27:14 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 48DF261041 for ; Mon, 26 Apr 2021 10:27:14 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232483AbhDZK1y (ORCPT ); Mon, 26 Apr 2021 06:27:54 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:49432 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232253AbhDZK1x (ORCPT ); Mon, 26 Apr 2021 06:27:53 -0400 Received: from mail-pg1-x52d.google.com (mail-pg1-x52d.google.com [IPv6:2607:f8b0:4864:20::52d]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 89494C061756 for ; Mon, 26 Apr 2021 03:27:12 -0700 (PDT) Received: by mail-pg1-x52d.google.com with SMTP id p12so39762066pgj.10 for ; Mon, 26 Apr 2021 03:27:12 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=fCq7GuwqxWrfP0zG2BPEk4vgQibkYkXRTf/6CONjF7w=; b=DTjN/6J2o556h9+s0z3fLOtkfNzVv9SCgICxXgIPLFX8+AoKgPqACuTq4k1VsONxd3 v0O4S1e5BGAGciKXqm+56etmpe5wqBOwGoDadVNYWw0tDJS3uhlsvL0zIl9dxJecAlcl KQx6qZVNw6m1pfJ3XTCiNogW9HWyGDykDW4diOPL5Gf6rikNjmBJoe04ADwnLyEghU+b aoidb7YDzGVE9CVYolLZ5Ky4Keeq5HA8RmUbzL0+Q7+7Z3gjog6SuRbOV9rK8oJQQZO7 Op6lzhbYUH5PIT97XQHxWRUHhf3Kb7DAcHL7xqDwlaHBAINiEDWiUXqqgq8PKuspd9XV XCig== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=fCq7GuwqxWrfP0zG2BPEk4vgQibkYkXRTf/6CONjF7w=; b=JNEmcY67yGc3y/R6bn20B1HJnM1rM8WjXsM7Qp1c+1EXMCjHaRwduUuM4EpXxm10DE rltV14bx/X0p4h9khHqNZDlm8dkArpbdebjh4LomZm3tSgGceeNBNul6n2tPNtnhRxrX l0ZFYacBzKiYTm8jKE9y8vliztH1Y8H6AS4oxYgKZmTrIGP3UALkQpZZCqZy16uspUpT y0Y94m0Dpobl1pUUKRcakytWP57cM2mTcZSHX3EwE+Zv39FhxBehqyoTlHdUxfuBu8Ax bL6XSXtm26BuNpgQZH3DDc12EyGN6Nt/fHK3OK2u7VJonxW1iKrD/Rftt35b9hKwqKj1 wPIg== X-Gm-Message-State: AOAM531aCwwnduJ6/2+MV+XR/ed840j5ToV2O6qO6ZkZz7WMU1S2xCpj Uzc2+EFjnhmMNAG1Frfzg0RZnQ== X-Google-Smtp-Source: ABdhPJyJR0whQZLY4qriE+qC8b4jR9IpHivd9Pv/u7Gb9sW4QiBEtei9pm7ucR/kZZmImf3ROb4hcg== X-Received: by 2002:a63:5c19:: with SMTP id q25mr16710460pgb.402.1619432831858; Mon, 26 Apr 2021 03:27:11 -0700 (PDT) Received: from google.com ([2401:fa00:9:211:5345:1783:3859:c0bf]) by smtp.gmail.com with ESMTPSA id f71sm5219603pfa.91.2021.04.26.03.27.09 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 26 Apr 2021 03:27:11 -0700 (PDT) Date: Mon, 26 Apr 2021 20:26:53 +1000 From: Matthew Bobrowski To: Amir Goldstein Cc: Jan Kara , Christian Brauner , Linux API , linux-fsdevel Subject: Re: [PATCH 2/2] fanotify: Add pidfd support to the fanotify API Message-ID: References: <20210419132020.ydyb2ly6e3clhe2j@wittgenstein> <20210419135550.GH8706@quack2.suse.cz> <20210419150233.rgozm4cdbasskatk@wittgenstein> <20210421080449.GK8706@quack2.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-api@vger.kernel.org On Fri, Apr 23, 2021 at 11:14:34AM +0300, Amir Goldstein wrote: > On Fri, Apr 23, 2021 at 11:02 AM Matthew Bobrowski wrote: > > > > On Fri, Apr 23, 2021 at 10:39:46AM +0300, Amir Goldstein wrote: > > > On Fri, Apr 23, 2021 at 2:06 AM Matthew Bobrowski wrote: > > > > > > > > On Wed, Apr 21, 2021 at 10:04:49AM +0200, Jan Kara wrote: > > > > > 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 <>, 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. > > > > > > > > This approach makes sense to me. > > > > > > > > Jan/Amir, just to be clear, we've agreed to go ahead with the extended > > > > struct approach whereby specifying the FAN_REPORT_PIDFD flag will > > > > result in an event which includes an additional struct > > > > (i.e. fanotify_event_info_pid) alongside the generic existing > > > > > > struct fanotify_event_info_pidfd? > > > > Well, yeah? I mean, my line of thought was that we'd also need to > > include struct fanotify_event_info_header alongside the event to > > provide more meta-information about the additional event you'd expect > > to receive when FAN_REPORT_PIDFD is provided, so we'd end up with > > something like: > > > > struct fanotify_event_info_pidfd { > > struct fanotify_event_info_header hdr; > > __s32 pidfd; > > } > > > > Unless this of course is overbaking it and there's no need to do this? > > > > We need this. I was just pointing out that you wrote fanotify_event_info_pid > must have been a typo. Oh, right, that sure was a typo! :) Amir, I was just thinking about this a little over the weekend and I don't think we discussed how to handle the FAN_REPORT_PIDFD | FAN_REPORT_FID and friends case? My immediate thought is to make FAN_REPORT_PIDFD mutually exclusive with FAN_REPORT_FID and friends, but then again receiving a pidfd along with FID events may be also useful for some? What are your thoughts on this? If we don't go ahead with mutual exclusion, then this multiple event types alongside struct fanotify_event_metadata starts getting a little clunky, don't you think? /M