On 2019/01/18 12:52, Jason Wang wrote:

On 2019/1/18 上午9:56, Toshiaki Makita wrote:

quoted

On 2019/01/17 22:05, Jason Wang wrote:

quoted

On 2019/1/17 下午8:53, Jason Wang wrote:

quoted

On 2019/1/17 下午7:20, Toshiaki Makita wrote:

quoted

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 <redacted>

I wonder whether or not we could simply do:


if (prog) {

Should be !prog

quoted

     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.

quoted

---

But strictly speaking, virtnet_set_queues() should not be necessary if
(prog != NULL && old_prog != NULL).

Yes, but it was another possible 'issue'.

quoted

If you prefer this, I can modify it accordingly.

I prefer to do this change.

OK, will do in v2.

-- 
Toshiaki Makita