| 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
| ||
|
Message-ID: <17EC94B0A072C34B8DCF0D30AD16044A0287864B@BPXM09GP.gisp.nec.co.jp>
Date: Fri, 16 Oct 2015 01:46:48 +0000
From: Kosuke Tatsukawa <tatsu@...jp.nec.com>
To: "J. Bruce Fields" <bfields@...ldses.org>
CC: Trond Myklebust <trond.myklebust@...marydata.com>,
Neil Brown <nfbrown@...ell.com>,
Anna Schumaker <anna.schumaker@...app.com>,
Jeff Layton <jlayton@...chiereds.net>,
"David S. Miller" <davem@...emloft.net>,
"linux-nfs@...r.kernel.org" <linux-nfs@...r.kernel.org>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] sunrpc: fix waitqueue_active without memory barrier
in sunrpc
J. Bruce Fields wrote:
> On Thu, Oct 15, 2015 at 11:44:20AM +0000, Kosuke Tatsukawa wrote:
>> Tatsukawa Kosuke wrote:
>> > J. Bruce Fields wrote:
>> >> Thanks for the detailed investigation.
>> >>
>> >> I think it would be worth adding a comment if that might help someone
>> >> having to reinvestigate this again some day.
>> >
>> > It would be nice, but I find it difficult to write a comment in the
>> > sunrpc layer why a memory barrier isn't necessary, using the knowledge
>> > of how nfsd uses it, and the current implementation of the network code.
>> >
>> > Personally, I would prefer removing the call to waitqueue_active() which
>> > would make the memory barrier totally unnecessary at the cost of a
>> > spin_lock + spin_unlock by unconditionally calling
>> > wake_up_interruptible.
>>
>> On second thought, the callbacks will be called frequently from the tcp
>> code, so it wouldn't be a good idea.
>
> So, I was even considering documenting it like this, if it's not
> overkill.
>
> Hmm... but if this is right, then we may as well ask why we're doing the
> wakeups at all. Might be educational to test the code with them
> removed.
sk_write_space will be called in sock_wfree() with UDP/IP each time
kfree_skb() is called. With TCP/IP, sk_write_space is only called if
SOCK_NOSPACE has been set.
sk_data_ready will be called in both tcp_rcv_established() for TCP/IP
and in sock_queue_rcv_skb() for UDP/IP. The latter lacks a memory
barrier with sk_data_ready called right after __skb_queue_tail().
I think this hasn't caused any problems because sk_data_ready wasn't
used.
> --b.
>
> commit 0882cfeb39e0
> Author: J. Bruce Fields <bfields@...hat.com>
> Date: Thu Oct 15 16:53:41 2015 -0400
>
> svcrpc: document lack of some memory barriers.
>
> Kosuke Tatsukawa points out an odd lack of memory barriers in some sites
> here. I think the code's correct, but it's probably worth documenting.
>
> Reported-by: Kosuke Tatsukawa <tatsu@...jp.nec.com>
>
> diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
> index 856407fa085e..90480993ec4a 100644
> --- a/net/sunrpc/svcsock.c
> +++ b/net/sunrpc/svcsock.c
> @@ -399,6 +399,25 @@ static int svc_sock_secure_port(struct svc_rqst *rqstp)
> return svc_port_is_privileged(svc_addr(rqstp));
> }
>
> +static void svc_no_smp_mb(void)
> +{
> + /*
> + * Kosuke Tatsukawa points out there should normally be an
> + * smp_mb() at the callsites of this function. (Either that or
> + * we could just drop the waitqueue_active() checks.)
> + *
> + * It appears they aren't currently necessary, though, basically
> + * because nfsd does non-blocking reads from these sockets, so
> + * the only places we wait on this waitqueue is in sendpage and
> + * sendmsg, which won't be waiting for wakeups on newly arrived
> + * data.
> + *
> + * Maybe we should add the memory barriers anyway, but these are
> + * hot paths so we'd need to be convinced there's no sigificant
> + * penalty.
> + */
> +}
> +
> /*
> * INET callback when data has been received on the socket.
> */
> @@ -414,7 +433,7 @@ static void svc_udp_data_ready(struct sock *sk)
> set_bit(XPT_DATA, &svsk->sk_xprt.xpt_flags);
> svc_xprt_enqueue(&svsk->sk_xprt);
> }
> - smp_mb();
> + svc_no_smp_mb();
> if (wq && waitqueue_active(wq))
> wake_up_interruptible(wq);
> }
> @@ -433,7 +452,7 @@ static void svc_write_space(struct sock *sk)
> svc_xprt_enqueue(&svsk->sk_xprt);
> }
>
> - smp_mb();
> + svc_no_smp_mb();
> if (wq && waitqueue_active(wq)) {
> dprintk("RPC svc_write_space: someone sleeping on %p\n",
> svsk);
> @@ -789,7 +808,7 @@ static void svc_tcp_listen_data_ready(struct sock *sk)
> }
>
> wq = sk_sleep(sk);
> - smp_mb();
> + svc_no_smp_mb();
> if (wq && waitqueue_active(wq))
> wake_up_interruptible_all(wq);
> }
> @@ -811,7 +830,7 @@ static void svc_tcp_state_change(struct sock *sk)
> set_bit(XPT_CLOSE, &svsk->sk_xprt.xpt_flags);
> svc_xprt_enqueue(&svsk->sk_xprt);
> }
> - smp_mb();
> + svc_no_smp_mb();
> if (wq && waitqueue_active(wq))
> wake_up_interruptible_all(wq);
> }
> @@ -827,7 +846,7 @@ static void svc_tcp_data_ready(struct sock *sk)
> set_bit(XPT_DATA, &svsk->sk_xprt.xpt_flags);
> svc_xprt_enqueue(&svsk->sk_xprt);
> }
> - smp_mb();
> + svc_no_smp_mb();
> if (wq && waitqueue_active(wq))
> wake_up_interruptible(wq);
> }
> @@ -1599,7 +1618,7 @@ static void svc_sock_detach(struct svc_xprt *xprt)
> sk->sk_write_space = svsk->sk_owspace;
>
> wq = sk_sleep(sk);
> - smp_mb();
> + svc_no_smp_mb();
> if (wq && waitqueue_active(wq))
> wake_up_interruptible(wq);
> }
---
Kosuke TATSUKAWA | 3rd IT Platform Department
| IT Platform Division, NEC Corporation
| tatsu@...jp.nec.com
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists