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=-4.0 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SIGNED_OFF_BY,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 3D12DC43387 for ; Fri, 18 Jan 2019 03:53:12 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 0FFED20850 for ; Fri, 18 Jan 2019 03:53:12 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726999AbfARDxK (ORCPT ); Thu, 17 Jan 2019 22:53:10 -0500 Received: from mx1.redhat.com ([209.132.183.28]:60772 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726834AbfARDxK (ORCPT ); Thu, 17 Jan 2019 22:53:10 -0500 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id D01B27BDC5; Fri, 18 Jan 2019 03:53:09 +0000 (UTC) Received: from [10.72.12.162] (ovpn-12-162.pek2.redhat.com [10.72.12.162]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 642A71048114; Fri, 18 Jan 2019 03:52:55 +0000 (UTC) Subject: Re: [PATCH net 5/7] virtio_net: Don't process redirected XDP frames when XDP is disabled To: Toshiaki Makita , "David S. Miller" , "Michael S. Tsirkin" Cc: netdev@vger.kernel.org, virtualization@lists.linux-foundation.org, Jesper Dangaard Brouer References: <1547724045-2726-1-git-send-email-makita.toshiaki@lab.ntt.co.jp> <1547724045-2726-6-git-send-email-makita.toshiaki@lab.ntt.co.jp> <24359f65-fe45-46d1-5344-f0cc4705009b@redhat.com> From: Jason Wang Message-ID: Date: Fri, 18 Jan 2019 11:52:51 +0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-US X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.26]); Fri, 18 Jan 2019 03:53:10 +0000 (UTC) Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On 2019/1/18 上午9:56, Toshiaki Makita wrote: > On 2019/01/17 22:05, Jason Wang wrote: >> On 2019/1/17 下午8:53, Jason Wang wrote: >>> On 2019/1/17 下午7:20, Toshiaki Makita wrote: >>>> Commit 8dcc5b0ab0ec ("virtio_net: fix ndo_xdp_xmit crash towards dev not >>>> ready for XDP") tried to avoid access to unexpected sq while XDP is >>>> disabled, but was not complete. >>>> >>>> There was a small window which causes out of bounds sq access in >>>> virtnet_xdp_xmit() while disabling XDP. >>>> >>>> An example case of >>>>   - curr_queue_pairs = 6 (2 for SKB and 4 for XDP) >>>>   - online_cpu_num = xdp_queue_paris = 4 >>>> when XDP is enabled: >>>> >>>> CPU 0                         CPU 1 >>>> (Disabling XDP)               (Processing redirected XDP frames) >>>> >>>>                                virtnet_xdp_xmit() >>>> virtnet_xdp_set() >>>>   _virtnet_set_queues() >>>>    set curr_queue_pairs (2) >>>>                                 check if rq->xdp_prog is not NULL >>>>                                 virtnet_xdp_sq(vi) >>>>                                  qp = curr_queue_pairs - >>>>                                       xdp_queue_pairs + >>>>                                       smp_processor_id() >>>>                                     = 2 - 4 + 1 = -1 >>>>                                  sq = &vi->sq[qp] // out of bounds >>>> access >>>>    set xdp_queue_pairs (0) >>>>    rq->xdp_prog = NULL >>>> >>>> Basically we should not change curr_queue_pairs and xdp_queue_pairs >>>> while someone can read the values. Thus, when disabling XDP, assign NULL >>>> to rq->xdp_prog first, and wait for RCU grace period, then change >>>> xxx_queue_pairs. >>>> Note that we need to keep the current order when enabling XDP though. >>>> >>>> Fixes: 186b3c998c50 ("virtio-net: support XDP_REDIRECT") >>>> Signed-off-by: Toshiaki Makita >>> >>> I wonder whether or not we could simply do: >>> >>> >>> if (prog) { >> >> Should be !prog >> >> >>>     rcu_assign_pointer() >>> >>>     synchronize_net() >>> >>> } >>> >>> set queues >>> >>> if (!prog) { >> >> Should be prog. > Either would work. > > With your suggestion the code will look like: > > --- > if (!prog) { > for (...) { > rcu_assign_pointer(); > ... > } > synchronize_net(); > } > > virtnet_set_queues(); > netif_set_real_num_rx_queues(); > vi->xdp_queue_pairs = xdp_qp; > > if (prog) { > for (...) { > rcu_assign_pointer(); > ... > } > } Yes, I think this makes code more easier to be understand. > --- > > But strictly speaking, virtnet_set_queues() should not be necessary if > (prog != NULL && old_prog != NULL). Yes, but it was another possible 'issue'. > If you prefer this, I can modify it accordingly. I prefer to do this change. Thanks