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=-12.7 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable 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 6329CC433B4 for ; Mon, 19 Apr 2021 13:53:47 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 189F761279 for ; Mon, 19 Apr 2021 13:53:47 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230063AbhDSNyP (ORCPT ); Mon, 19 Apr 2021 09:54:15 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:38170 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229635AbhDSNyP (ORCPT ); Mon, 19 Apr 2021 09:54:15 -0400 Received: from mail-io1-xd32.google.com (mail-io1-xd32.google.com [IPv6:2607:f8b0:4864:20::d32]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 54D20C06174A; Mon, 19 Apr 2021 06:53:44 -0700 (PDT) Received: by mail-io1-xd32.google.com with SMTP id e186so34933665iof.7; Mon, 19 Apr 2021 06:53:44 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=OPv6D6qV3tPMcXiH4gaypxFmGbYasaTtXFMZiPqVoz0=; b=hGYbCPF3mcgLHYPsq/L8z2yBpwrgufg6YB5/W7gkdBc0HRUDVIqwcW0IqxgnS4n6za LAp2Ak2Z0sDL2g83auIyVE9Wef83VYnR1QY0DNFZ9GCo3Trhy5CzCPXNjYMNt5LO7EQ+ ITxJLskPv0PZtb/eM/+x2S4PlgUK7y1u/D6+kk9NvXURlvMQmQmLb3V8fPjy6PjSrahk H8q2EM5Nva2kF6gmsawzxvQVyIupSWluwdLg1BUBdU5+erQ/IeTf+ZRPOjhxxkA3VSAu nJaxG/GFD228oKkp9Tn6omUeeDmIPX/tddn5xDgNpHL2abCQrup8i9ECpzsmW7gNq2yp 6KNQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=OPv6D6qV3tPMcXiH4gaypxFmGbYasaTtXFMZiPqVoz0=; b=i1H/pNLAfHo+F2nvfzz+YXITYToKNpae6NhcrvEIVdosXLaGwinjeUv35W0Y2yKIcp U3W0uuysZydWa5KF1xcrTHCJzsg/T9A6VImDMJWuistHCalvYZTvagdExmWclb8rISQc tQpUngEhtBxyy5m2J/fUzK+68XeeCZv6oniwPZ+Y0ec1UHhtqeH1Uy3GoxomfzM00HkB j/nFDZs1Ex7aNPjXT361gTrnMSC/vPpzSgukQKXKfGCNeMA3QkOnyXELqP/KthKJE++U Qam9rlD3xgPE9df/8m7OATqcm6c7mvolPyMw5yBWNfn19FsUFcZF0BUMbJ8FCu085Muh xQgw== X-Gm-Message-State: AOAM531oV6qPMw7GTkbbVIaiLeQuJ+Yu58wmOvxthuO99SSyt9MRVxpA nbcGs9dJgAQDLl9WzrFL0o3OzXzrZYvyfjHIftF6c44wyfQ= X-Google-Smtp-Source: ABdhPJwF8Y7H/Jxkbi0MQA3m2KH+O7fTjxhxOQpdyD1v1HhmDipRiloZ30/OwPcjn02QgBb2TBq5Vvb9eDremcm3KRI= X-Received: by 2002:a6b:f115:: with SMTP id e21mr2617155iog.5.1618840423784; Mon, 19 Apr 2021 06:53:43 -0700 (PDT) MIME-Version: 1.0 References: <20210419132020.ydyb2ly6e3clhe2j@wittgenstein> In-Reply-To: <20210419132020.ydyb2ly6e3clhe2j@wittgenstein> From: Amir Goldstein Date: Mon, 19 Apr 2021 16:53:32 +0300 Message-ID: Subject: Re: [PATCH 2/2] fanotify: Add pidfd support to the fanotify API To: Christian Brauner Cc: Matthew Bobrowski , Jan Kara , Linux API , linux-fsdevel Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-fsdevel@vger.kernel.org On Mon, Apr 19, 2021 at 4:20 PM Christian Brauner wrote: > > On Fri, Apr 16, 2021 at 09:22:25AM +1000, Matthew Bobrowski wrote: > > Introduce a new flag FAN_REPORT_PIDFD for fanotify_init(2) which > > allows userspace applications to control whether a pidfd is to be > > returned instead of a pid for `struct fanotify_event_metadata.pid`. > > > > FAN_REPORT_PIDFD is mutually exclusive with FAN_REPORT_TID as the > > pidfd API is currently restricted to only support pidfd generation for > > thread-group leaders. Attempting to set them both when calling > > fanotify_init(2) will result in -EINVAL being returned to the > > caller. As the pidfd API evolves and support is added for tids, this > > is something that could be relaxed in the future. > > > > If pidfd creation fails, the pid in struct fanotify_event_metadata is > > set to FAN_NOPIDFD(-1). Falling back and providing a pid instead of a > > pidfd on pidfd creation failures was considered, although this could > > possibly lead to confusion and unpredictability within userspace > > applications as distinguishing between whether an actual pidfd or pid > > was returned could be difficult, so it's best to be explicit. > > > > Signed-off-by: Matthew Bobrowski > > --- > > fs/notify/fanotify/fanotify_user.c | 33 +++++++++++++++++++++++++++--- > > include/linux/fanotify.h | 2 +- > > include/uapi/linux/fanotify.h | 2 ++ > > 3 files changed, 33 insertions(+), 4 deletions(-) > > > > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c > > index 9e0c1afac8bd..fd8ae88796a8 100644 > > --- a/fs/notify/fanotify/fanotify_user.c > > +++ b/fs/notify/fanotify/fanotify_user.c > > @@ -329,7 +329,7 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group, > > struct fanotify_info *info = fanotify_event_info(event); > > unsigned int fid_mode = FAN_GROUP_FLAG(group, FANOTIFY_FID_BITS); > > struct file *f = NULL; > > - int ret, fd = FAN_NOFD; > > + int ret, pidfd, fd = FAN_NOFD; > > int info_type = 0; > > > > pr_debug("%s: group=%p event=%p\n", __func__, group, event); > > @@ -340,7 +340,25 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group, > > metadata.vers = FANOTIFY_METADATA_VERSION; > > metadata.reserved = 0; > > metadata.mask = event->mask & FANOTIFY_OUTGOING_EVENTS; > > - metadata.pid = pid_vnr(event->pid); > > + > > + if (FAN_GROUP_FLAG(group, FAN_REPORT_PIDFD) && > > + pid_has_task(event->pid, PIDTYPE_TGID)) { > > + /* > > + * Given FAN_REPORT_PIDFD is to be mutually exclusive with > > + * FAN_REPORT_TID, panic here if the mutual exclusion is ever > > + * blindly lifted without pidfds for threads actually being > > + * supported. > > + */ > > + WARN_ON(FAN_GROUP_FLAG(group, FAN_REPORT_TID)); > > + > > + pidfd = pidfd_create(event->pid, 0); > > + if (unlikely(pidfd < 0)) > > + metadata.pid = FAN_NOPIDFD; > > + else > > + metadata.pid = pidfd; > > I'm not a fan of overloading fields (Yes, we did this for the _legacy_ > clone() syscall for CLONE_PIDFD/CLONE_PARENT_SETTID but in general it's > never a good idea if there are other options, imho.). > Could/should we consider the possibility of adding a new pidfd field to > struct fanotify_event_metadata? struct fanotify_event_metadata is fully booked. We could use a variable length record, but IMO that's an overkill for pidfd. If you are concerned about users wrongly interpreting pidfd as pid and getting the the wrong process we could use invalid negative values for pidfd, e.g. metadata.pid = ~pidfd. This has the quality that 0 can mean both FAN_NOPIDFD and no pid. UAPI to this would be abstracted with #define FAN_EVENT_PIDFD(event) (~((event)->pid)) I am not convinced that this helps more than it hurts users, but abstracting the accessor to pidfd could be a good idea anyway. Thanks, Amir.