Re: [PATCH v2 net 5/7] virtio_net: Don't process redirected XDP frames when XDP is disabled

[Jason Wang <jasowang@redhat.com>]

On 2019/1/29 上午8:45, 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.
>
> - v2: Make rcu_assign_pointer/synchronize_net conditional instead of
>        _virtnet_set_queues.
>
> Fixes: 186b3c998c50 ("virtio-net: support XDP_REDIRECT")
> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> ---
>   drivers/net/virtio_net.c | 33 ++++++++++++++++++++++++++-------
>   1 file changed, 26 insertions(+), 7 deletions(-)


Acked-by: Jason Wang <jasowang@redhat.com>

Thanks.