linux-kernel - Re: [PATCH 1/4] seccomp: don't use semaphore and wait

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20220830095750.ljtdk7hekmmzhf2b@wittgenstein>
Date:   Tue, 30 Aug 2022 11:57:50 +0200
From:   Christian Brauner <brauner@...nel.org>
To:     Andrei Vagin <avagin@...il.com>
Cc:     linux-kernel@...r.kernel.org,
        Andy Lutomirski <luto@...capital.net>,
        Dietmar Eggemann <dietmar.eggemann@....com>,
        Kees Cook <keescook@...omium.org>,
        Ingo Molnar <mingo@...hat.com>,
        Juri Lelli <juri.lelli@...hat.com>,
        Peter Oskolkov <posk@...gle.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Tycho Andersen <tycho@...ho.pizza>,
        Will Drewry <wad@...omium.org>,
        Vincent Guittot <vincent.guittot@...aro.org>
Subject: Re: [PATCH 1/4] seccomp: don't use semaphore and wait_queue together

On Mon, Aug 29, 2022 at 06:43:53PM -0700, Andrei Vagin wrote:
> Here is no reason to use two different primitives that do similar things.
> 
> Signed-off-by: Andrei Vagin <avagin@...il.com>
> ---

I dug into the old threads a bit and there was no specific reason given
why we used the sempahore afaict. It was there from the beginning and
the wait queue got added to support polling. Maybe I'm missing why we
need both but it's not obvious to me right now as well. So I tend to
agree that we should get rid of it.

>  kernel/seccomp.c | 38 ++++++++++++++++++++++++++------------
>  1 file changed, 26 insertions(+), 12 deletions(-)
> 
> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> index e9852d1b4a5e..667fd2d89464 100644
> --- a/kernel/seccomp.c
> +++ b/kernel/seccomp.c
> @@ -145,7 +145,7 @@ struct seccomp_kaddfd {
>   * @notifications: A list of struct seccomp_knotif elements.
>   */
>  struct notification {
> -	struct semaphore request;
> +	atomic_t requests;
>  	u64 next_id;
>  	struct list_head notifications;
>  };
> @@ -1116,7 +1116,7 @@ static int seccomp_do_user_notification(int this_syscall,
>  	list_add_tail(&n.list, &match->notif->notifications);
>  	INIT_LIST_HEAD(&n.addfd);
>  
> -	up(&match->notif->request);
> +	atomic_add(1, &match->notif->requests);

atomic_inc()?

>  	wake_up_poll(&match->wqh, EPOLLIN | EPOLLRDNORM);
>  
>  	/*
> @@ -1388,8 +1388,10 @@ static long seccomp_set_mode_strict(void)
>  #ifdef CONFIG_SECCOMP_FILTER
>  static void seccomp_notify_free(struct seccomp_filter *filter)
>  {
> -	kfree(filter->notif);
> -	filter->notif = NULL;
> +	struct notification *notif = filter->notif;
> +
> +	WRITE_ONCE(filter->notif, NULL);
> +	kfree_rcu(notif);
>  }
>  
>  static void seccomp_notify_detach(struct seccomp_filter *filter)
> @@ -1450,6 +1452,16 @@ find_notification(struct seccomp_filter *filter, u64 id)
>  	return NULL;
>  }
>  
> +static bool notify_wakeup(struct seccomp_filter *filter)
> +{
> +	bool ret;
> +
> +	rcu_read_lock();
> +	ret = atomic_add_unless(&filter->notif->requests, -1, 0);

Can you please spell out why the change to wait_event_interruptible()
below that calls notify_wakeup() makes it necessary to rcu protect
->notif?

Given that you're using rcu_read_lock() here and the
WRITE_ONCE(fitler->notify, NULL) + kfree_rcu() sequence in
seccomp_notify_free() it seems to imply that filter->notif could be NULL
here and so would need a non-NULL check on ->notif before incrementing?

> +	rcu_read_unlock();
> +
> +	return ret;
> +}
>  
>  static long seccomp_notify_recv(struct seccomp_filter *filter,
>  				void __user *buf)
> @@ -1467,7 +1479,7 @@ static long seccomp_notify_recv(struct seccomp_filter *filter,
>  
>  	memset(&unotif, 0, sizeof(unotif));
>  
> -	ret = down_interruptible(&filter->notif->request);
> +	ret = wait_event_interruptible(filter->wqh, notify_wakeup(filter));
>  	if (ret < 0)
>  		return ret;
>  
> @@ -1515,7 +1527,8 @@ static long seccomp_notify_recv(struct seccomp_filter *filter,
>  			if (should_sleep_killable(filter, knotif))
>  				complete(&knotif->ready);
>  			knotif->state = SECCOMP_NOTIFY_INIT;
> -			up(&filter->notif->request);
> +			atomic_add(1, &filter->notif->requests);

atomic_inc()?

> +			wake_up_poll(&filter->wqh, EPOLLIN | EPOLLRDNORM);
>  		}
>  		mutex_unlock(&filter->notify_lock);
>  	}
> @@ -1771,15 +1784,15 @@ static const struct file_operations seccomp_notify_ops = {
>  static struct file *init_listener(struct seccomp_filter *filter)
>  {
>  	struct file *ret;
> +	struct notification *notif;
>  
>  	ret = ERR_PTR(-ENOMEM);
> -	filter->notif = kzalloc(sizeof(*(filter->notif)), GFP_KERNEL);
> -	if (!filter->notif)
> +	notif = kzalloc(sizeof(*notif), GFP_KERNEL);
> +	if (!notif)
>  		goto out;
>  
> -	sema_init(&filter->notif->request, 0);
> -	filter->notif->next_id = get_random_u64();
> -	INIT_LIST_HEAD(&filter->notif->notifications);
> +	notif->next_id = get_random_u64();
> +	INIT_LIST_HEAD(&notif->notifications);
>  
>  	ret = anon_inode_getfile("seccomp notify", &seccomp_notify_ops,
>  				 filter, O_RDWR);
> @@ -1788,10 +1801,11 @@ static struct file *init_listener(struct seccomp_filter *filter)
>  
>  	/* The file has a reference to it now */
>  	__get_seccomp_filter(filter);
> +	WRITE_ONCE(filter->notif, notif);
>  
>  out_notif:
>  	if (IS_ERR(ret))
> -		seccomp_notify_free(filter);
> +		kfree(notif);
>  out:
>  	return ret;
>  }
> -- 
> 2.37.2
> 
>