netdev - Re: Use of RCU in qrtr

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>] [day] [month] [year] [list]

Message-ID: <20200605180821.GA5421@bombadil.infradead.org>
Date:   Fri, 5 Jun 2020 11:08:21 -0700
From:   Matthew Wilcox <willy@...radead.org>
To:     Bjorn Andersson <bjorn.andersson@...aro.org>
Cc:     netdev@...r.kernel.org
Subject: Re: Use of RCU in qrtr


[I meant to cc netdev on this originally.  Added now, preserving entire
message below for context]

On Fri, Jun 05, 2020 at 05:12:05AM -0700, Matthew Wilcox wrote:
> While doing the XArray conversion, I came across your commit
> f16a4b26f31f95dddb12cf3c2390906a735203ae
> 
> synchronize_rcu() is kind of expensive on larger systems.  Is there a
> reason to call it instead of using RCU to free the socket?
> 
> I don't know whether I've set the flag in the right place, but maybe
> something like this would be a good idea?
> 
> @@ -663,13 +663,8 @@ static void qrtr_port_remove(struct qrtr_sock *ipc)
>         if (port == QRTR_PORT_CTRL)
>                 port = 0;
>  
> -       __sock_put(&ipc->sk);
> -
>         xa_erase(&qrtr_ports, port);
> -
> -       /* Ensure that if qrtr_port_lookup() did enter the RCU read section we
> -        * wait for it to up increment the refcount */
> -       synchronize_rcu();
> +       __sock_put(&ipc->sk);
>  }
>  
>  /* Assign port number to socket.
> @@ -749,6 +744,7 @@ static int __qrtr_bind(struct socket *sock,
>                 qrtr_port_remove(ipc);
>         ipc->us.sq_port = port;
>  
> +       sock_set_flag(sk, SOCK_RCU_FREE);
>         sock_reset_flag(sk, SOCK_ZAPPED);
>  
>         /* Notify all open ports about the new controller */

I realised this isn't sufficient.  If we want to eliminate the
synchronize_rcu() call, we need to have a 'try' variant of sock_hold().
It would look something like this:

static inline __must_check bool sock_try_get(struct sock *sk)
{
	return refcount_inc_not_zero(&sk->sk_refcnt);
}

and then ...

        rcu_read_lock();
        ipc = xa_load(&qrtr_ports, port);
        if (ipc && !sock_try_get(&ipc->sk))
		ipc = NULL;
        rcu_read_unlock();

If we wanted to be able to atomically replace a pointer in the qrtr_ports
array with another without ever seeing NULL in between, we'd want to
make this:

	rcu_read_lock():
	do {
		ipc = xa_load(&qrtr_ports, port);
	} while (ipc && !sock_try_get(&ipc->sk));
	rcu_read_unlock();

but as far as I can tell, we don't need to do that for qrtr.