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=-8.2 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED,USER_AGENT_SANE_1 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 E14CDC3A589 for ; Tue, 20 Aug 2019 21:24:35 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id BC09122DD3 for ; Tue, 20 Aug 2019 21:24:35 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1730900AbfHTVYc (ORCPT ); Tue, 20 Aug 2019 17:24:32 -0400 Received: from www62.your-server.de ([213.133.104.62]:55030 "EHLO www62.your-server.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728283AbfHTVYc (ORCPT ); Tue, 20 Aug 2019 17:24:32 -0400 Received: from sslproxy01.your-server.de ([88.198.220.130]) by www62.your-server.de with esmtpsa (TLSv1.2:DHE-RSA-AES256-GCM-SHA384:256) (Exim 4.89_1) (envelope-from ) id 1i0Bc5-0007QV-Pz; Tue, 20 Aug 2019 23:24:21 +0200 Received: from [178.197.249.40] (helo=pc-63.home) by sslproxy01.your-server.de with esmtpsa (TLSv1.2:ECDHE-RSA-AES256-GCM-SHA384:256) (Exim 4.89) (envelope-from ) id 1i0Bc5-0006tF-Df; Tue, 20 Aug 2019 23:24:21 +0200 Subject: Re: [PATCH bpf-next] xsk: proper socket state check in xsk_poll To: =?UTF-8?B?QmrDtnJuIFTDtnBlbA==?= Cc: syzbot+c82697e3043781e08802@syzkaller.appspotmail.com, Alexei Starovoitov , Netdev , =?UTF-8?B?QmrDtnJuIFTDtnBlbA==?= , bpf , David Miller , Jesper Dangaard Brouer , Jakub Kicinski , John Fastabend , Jonathan Lemon , Martin KaFai Lau , LKML , "Karlsson, Magnus" , Song Liu , syzkaller-bugs@googlegroups.com, Xdp , Yonghong Song , hdanton@sina.com References: <0000000000009167320590823a8c@google.com> <20190820100405.25564-1-bjorn.topel@gmail.com> From: Daniel Borkmann Message-ID: Date: Tue, 20 Aug 2019 23:24:19 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.7.2 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-Authenticated-Sender: daniel@iogearbox.net X-Virus-Scanned: Clear (ClamAV 0.100.3/25547/Tue Aug 20 10:27:49 2019) Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On 8/20/19 5:29 PM, Björn Töpel wrote: > On Tue, 20 Aug 2019 at 16:30, Daniel Borkmann wrote: >> On 8/20/19 12:04 PM, Björn Töpel wrote: >>> From: Björn Töpel >>> >>> The poll() implementation for AF_XDP sockets did not perform the >>> proper state checks, prior accessing the socket umem. This patch fixes >>> that by performing a xsk_is_bound() check. >>> >>> Suggested-by: Hillf Danton >>> Reported-by: syzbot+c82697e3043781e08802@syzkaller.appspotmail.com >>> Fixes: 77cd0d7b3f25 ("xsk: add support for need_wakeup flag in AF_XDP rings") >>> Signed-off-by: Björn Töpel >>> --- >>> net/xdp/xsk.c | 14 ++++++++++++-- >>> 1 file changed, 12 insertions(+), 2 deletions(-) >>> >>> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c >>> index ee4428a892fa..08bed5e92af4 100644 >>> --- a/net/xdp/xsk.c >>> +++ b/net/xdp/xsk.c >>> @@ -356,13 +356,20 @@ static int xsk_generic_xmit(struct sock *sk, struct msghdr *m, >>> return err; >>> } >>> >>> +static bool xsk_is_bound(struct xdp_sock *xs) >>> +{ >>> + struct net_device *dev = READ_ONCE(xs->dev); >>> + >>> + return dev && xs->state == XSK_BOUND; >>> +} >>> + >>> static int xsk_sendmsg(struct socket *sock, struct msghdr *m, size_t total_len) >>> { >>> bool need_wait = !(m->msg_flags & MSG_DONTWAIT); >>> struct sock *sk = sock->sk; >>> struct xdp_sock *xs = xdp_sk(sk); >>> >>> - if (unlikely(!xs->dev)) >>> + if (unlikely(!xsk_is_bound(xs))) >>> return -ENXIO; >>> if (unlikely(!(xs->dev->flags & IFF_UP))) >>> return -ENETDOWN; >>> @@ -383,6 +390,9 @@ static unsigned int xsk_poll(struct file *file, struct socket *sock, >>> struct net_device *dev = xs->dev; >>> struct xdp_umem *umem = xs->umem; >>> >>> + if (unlikely(!xsk_is_bound(xs))) >>> + return mask; >>> + >>> if (umem->need_wakeup) >>> dev->netdev_ops->ndo_xsk_wakeup(dev, xs->queue_id, >>> umem->need_wakeup); >>> @@ -417,7 +427,7 @@ static void xsk_unbind_dev(struct xdp_sock *xs) >>> { >>> struct net_device *dev = xs->dev; >>> >>> - if (!dev || xs->state != XSK_BOUND) >>> + if (!xsk_is_bound(xs)) >>> return; >> >> I think I'm a bit confused by your READ_ONCE() usage. ;-/ I can see why you're >> using it in xsk_is_bound() above, but then at the same time all the other callbacks >> like xsk_poll() or xsk_unbind_dev() above have a struct net_device *dev = xs->dev >> right before the test. Could you elaborate? > > Yes, now I'm confused as well! Digging deeper... I believe there are a > couple of places in xsk.c that do not have > READ_ONCE/WRITE_ONCE-correctness. Various xdp_sock members are read > lock-less outside the control plane mutex (mutex member of struct > xdp_sock). This needs some re-work. I'll look into using the newly Right, so even in above two cases, the compiler could have refetched, e.g. dev variable could have first been NULL, but xsk_is_bound() later returns true. > introduced state member (with corresponding read/write barriers) for > this. > > I'll cook some patch(es) that address this, but first it sounds like I > need to reread [1] two, or three times. At least. ;-) > > > Thanks, > Björn > > > [1] https://lwn.net/Articles/793253/ > > >> Thanks, >> Daniel