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=-6.7 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED autolearn=ham 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 6CF44C433DF for ; Mon, 25 May 2020 13:26:17 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 5795D2070A for ; Mon, 25 May 2020 13:26:17 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2390609AbgEYN0R (ORCPT ); Mon, 25 May 2020 09:26:17 -0400 Received: from youngberry.canonical.com ([91.189.89.112]:37096 "EHLO youngberry.canonical.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S2388794AbgEYN0Q (ORCPT ); Mon, 25 May 2020 09:26:16 -0400 Received: from ip5f5af183.dynamic.kabel-deutschland.de ([95.90.241.131] helo=wittgenstein) by youngberry.canonical.com with esmtpsa (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.86_2) (envelope-from ) id 1jdD7M-0001k3-Eu; Mon, 25 May 2020 13:26:12 +0000 Date: Mon, 25 May 2020 15:26:11 +0200 From: Christian Brauner To: Sargun Dhillon Cc: linux-kernel@vger.kernel.org, containers@lists.linux-foundation.org, linux-api@vger.kernel.org, tycho@tycho.ws, keescook@chromium.org, cyphar@cyphar.com, Jeffrey Vander Stoep , jannh@google.com, rsesek@google.com, palmer@google.com, Matt Denton , Kees Cook Subject: Re: [PATCH 1/5] seccomp: Add find_notification helper Message-ID: <20200525132611.ltpkbe6ditxkag2r@wittgenstein> References: <20200524233942.8702-1-sargun@sargun.me> <20200524233942.8702-2-sargun@sargun.me> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200524233942.8702-2-sargun@sargun.me> Sender: linux-api-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-api@vger.kernel.org On Sun, May 24, 2020 at 04:39:38PM -0700, Sargun Dhillon wrote: > This adds a helper which can iterate through a seccomp_filter to > find a notification matching an ID. It removes several replicated > chunks of code. > > Signed-off-by: Sargun Dhillon > Cc: Matt Denton > Cc: Kees Cook , > Cc: Jann Horn , > Cc: Robert Sesek , > Cc: Chris Palmer > Cc: Christian Brauner > Cc: Tycho Andersen > --- > kernel/seccomp.c | 38 +++++++++++++++++++++----------------- > 1 file changed, 21 insertions(+), 17 deletions(-) > > diff --git a/kernel/seccomp.c b/kernel/seccomp.c > index 55a6184f5990..f6ce94b7a167 100644 > --- a/kernel/seccomp.c > +++ b/kernel/seccomp.c > @@ -1021,10 +1021,25 @@ static int seccomp_notify_release(struct inode *inode, struct file *file) > return 0; > } > > +/* must be called with notif_lock held */ > +static inline struct seccomp_knotif * > +find_notification(struct seccomp_filter *filter, u64 id) > +{ > + struct seccomp_knotif *cur; > + > + list_for_each_entry(cur, &filter->notif->notifications, list) { > + if (cur->id == id) > + return cur; > + } > + > + return NULL; > +} > + > + > static long seccomp_notify_recv(struct seccomp_filter *filter, > void __user *buf) > { > - struct seccomp_knotif *knotif = NULL, *cur; > + struct seccomp_knotif *knotif, *cur; > struct seccomp_notif unotif; > ssize_t ret; > > @@ -1078,14 +1093,8 @@ static long seccomp_notify_recv(struct seccomp_filter *filter, > * may have died when we released the lock, so we need to make > * sure it's still around. > */ > - knotif = NULL; > mutex_lock(&filter->notify_lock); > - list_for_each_entry(cur, &filter->notif->notifications, list) { > - if (cur->id == unotif.id) { > - knotif = cur; > - break; > - } > - } > + knotif = find_notification(filter, unotif.id); > > if (knotif) { > knotif->state = SECCOMP_NOTIFY_INIT; > @@ -1150,7 +1159,7 @@ static long seccomp_notify_send(struct seccomp_filter *filter, > static long seccomp_notify_id_valid(struct seccomp_filter *filter, > void __user *buf) > { > - struct seccomp_knotif *knotif = NULL; > + struct seccomp_knotif *knotif; > u64 id; > long ret; > > @@ -1162,15 +1171,10 @@ static long seccomp_notify_id_valid(struct seccomp_filter *filter, > return ret; > > ret = -ENOENT; > - list_for_each_entry(knotif, &filter->notif->notifications, list) { > - if (knotif->id == id) { > - if (knotif->state == SECCOMP_NOTIFY_SENT) > - ret = 0; > - goto out; > - } > - } > + knotif = find_notification(filter, id); > + if (knotif && knotif->state == SECCOMP_NOTIFY_SENT) > + ret = 0; Coul be a little nicer to have this be: if (knotif && knotif->state == SECCOMP_NOTIFY_SENT) ret = 0; else ret = -ENOENT; or, if you want to keep the assignment out of the lock: ret = -ENOENT; ret = mutex_lock_interruptible(&filter->notify_lock); if (ret < 0) return ret; knotif = find_notification(filter, id); if (knotif && knotif->state == SECCOMP_NOTIFY_SENT) ret = 0; otherwise looks like a good cleanup to me.