| Message ID | 1547724045-2726-6-git-send-email-makita.toshiaki@lab.ntt.co.jp |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | virtio_net: Fix problems around XDP tx and napi_tx | expand |
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 <makita.toshiaki@lab.ntt.co.jp> I wonder whether or not we could simply do: if (prog) { rcu_assign_pointer() synchronize_net() } set queues if (!prog) { rcu_assign_pointer() } Thanks > --- > drivers/net/virtio_net.c | 32 +++++++++++++++++++++++++------- > 1 file changed, 25 insertions(+), 7 deletions(-) > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > index 204eedf..ae93f0e 100644 > --- a/drivers/net/virtio_net.c > +++ b/drivers/net/virtio_net.c > @@ -2424,14 +2424,16 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog, > } > } > > - err = _virtnet_set_queues(vi, curr_qp + xdp_qp); > - if (err) > - goto err; > - netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp); > - vi->xdp_queue_pairs = xdp_qp; > + old_prog = rtnl_dereference(vi->rq[0].xdp_prog); > + if (!old_prog && prog) { > + err = _virtnet_set_queues(vi, curr_qp + xdp_qp); > + if (err) > + goto err_new_prog; > + netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp); > + vi->xdp_queue_pairs = xdp_qp; > + } > > for (i = 0; i < vi->max_queue_pairs; i++) { > - old_prog = rtnl_dereference(vi->rq[i].xdp_prog); > rcu_assign_pointer(vi->rq[i].xdp_prog, prog); > if (i == 0) { > if (!old_prog) > @@ -2439,6 +2441,18 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog, > if (!prog) > virtnet_restore_guest_offloads(vi); > } > + } > + > + if (old_prog && !prog) { > + synchronize_net(); > + err = _virtnet_set_queues(vi, curr_qp + xdp_qp); > + if (err) > + goto err_old_prog; > + netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp); > + vi->xdp_queue_pairs = xdp_qp; > + } > + > + for (i = 0; i < vi->max_queue_pairs; i++) { > if (old_prog) > bpf_prog_put(old_prog); > if (netif_running(dev)) { > @@ -2450,7 +2464,11 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog, > > return 0; > > -err: > +err_old_prog: > + virtnet_clear_guest_offloads(vi); > + for (i = 0; i < vi->max_queue_pairs; i++) > + rcu_assign_pointer(vi->rq[i].xdp_prog, old_prog); > +err_new_prog: > if (netif_running(dev)) { > for (i = 0; i < vi->max_queue_pairs; i++) { > virtnet_napi_enable(vi->rq[i].vq, &vi->rq[i].napi);
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 <makita.toshiaki@lab.ntt.co.jp> > > > 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. Thanks > > rcu_assign_pointer() > > } > > Thanks > > >> --- >> drivers/net/virtio_net.c | 32 +++++++++++++++++++++++++------- >> 1 file changed, 25 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c >> index 204eedf..ae93f0e 100644 >> --- a/drivers/net/virtio_net.c >> +++ b/drivers/net/virtio_net.c >> @@ -2424,14 +2424,16 @@ static int virtnet_xdp_set(struct net_device >> *dev, struct bpf_prog *prog, >> } >> } >> - err = _virtnet_set_queues(vi, curr_qp + xdp_qp); >> - if (err) >> - goto err; >> - netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp); >> - vi->xdp_queue_pairs = xdp_qp; >> + old_prog = rtnl_dereference(vi->rq[0].xdp_prog); >> + if (!old_prog && prog) { >> + err = _virtnet_set_queues(vi, curr_qp + xdp_qp); >> + if (err) >> + goto err_new_prog; >> + netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp); >> + vi->xdp_queue_pairs = xdp_qp; >> + } >> for (i = 0; i < vi->max_queue_pairs; i++) { >> - old_prog = rtnl_dereference(vi->rq[i].xdp_prog); >> rcu_assign_pointer(vi->rq[i].xdp_prog, prog); >> if (i == 0) { >> if (!old_prog) >> @@ -2439,6 +2441,18 @@ static int virtnet_xdp_set(struct net_device >> *dev, struct bpf_prog *prog, >> if (!prog) >> virtnet_restore_guest_offloads(vi); >> } >> + } >> + >> + if (old_prog && !prog) { >> + synchronize_net(); >> + err = _virtnet_set_queues(vi, curr_qp + xdp_qp); >> + if (err) >> + goto err_old_prog; >> + netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp); >> + vi->xdp_queue_pairs = xdp_qp; >> + } >> + >> + for (i = 0; i < vi->max_queue_pairs; i++) { >> if (old_prog) >> bpf_prog_put(old_prog); >> if (netif_running(dev)) { >> @@ -2450,7 +2464,11 @@ static int virtnet_xdp_set(struct net_device >> *dev, struct bpf_prog *prog, >> return 0; >> -err: >> +err_old_prog: >> + virtnet_clear_guest_offloads(vi); >> + for (i = 0; i < vi->max_queue_pairs; i++) >> + rcu_assign_pointer(vi->rq[i].xdp_prog, old_prog); >> +err_new_prog: >> if (netif_running(dev)) { >> for (i = 0; i < vi->max_queue_pairs; i++) { >> virtnet_napi_enable(vi->rq[i].vq, &vi->rq[i].napi);
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 <makita.toshiaki@lab.ntt.co.jp> >> >> >> 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(); ... } } --- But strictly speaking, virtnet_set_queues() should not be necessary if (prog != NULL && old_prog != NULL). If you prefer this, I can modify it accordingly.
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 <makita.toshiaki@lab.ntt.co.jp> >>> >>> 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
On 2019/01/18 12:52, Jason Wang wrote: > 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 <makita.toshiaki@lab.ntt.co.jp> >>>> >>>> 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. OK, will do in v2.
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index 204eedf..ae93f0e 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -2424,14 +2424,16 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog, } } - err = _virtnet_set_queues(vi, curr_qp + xdp_qp); - if (err) - goto err; - netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp); - vi->xdp_queue_pairs = xdp_qp; + old_prog = rtnl_dereference(vi->rq[0].xdp_prog); + if (!old_prog && prog) { + err = _virtnet_set_queues(vi, curr_qp + xdp_qp); + if (err) + goto err_new_prog; + netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp); + vi->xdp_queue_pairs = xdp_qp; + } for (i = 0; i < vi->max_queue_pairs; i++) { - old_prog = rtnl_dereference(vi->rq[i].xdp_prog); rcu_assign_pointer(vi->rq[i].xdp_prog, prog); if (i == 0) { if (!old_prog) @@ -2439,6 +2441,18 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog, if (!prog) virtnet_restore_guest_offloads(vi); } + } + + if (old_prog && !prog) { + synchronize_net(); + err = _virtnet_set_queues(vi, curr_qp + xdp_qp); + if (err) + goto err_old_prog; + netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp); + vi->xdp_queue_pairs = xdp_qp; + } + + for (i = 0; i < vi->max_queue_pairs; i++) { if (old_prog) bpf_prog_put(old_prog); if (netif_running(dev)) { @@ -2450,7 +2464,11 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog, return 0; -err: +err_old_prog: + virtnet_clear_guest_offloads(vi); + for (i = 0; i < vi->max_queue_pairs; i++) + rcu_assign_pointer(vi->rq[i].xdp_prog, old_prog); +err_new_prog: if (netif_running(dev)) { for (i = 0; i < vi->max_queue_pairs; i++) { virtnet_napi_enable(vi->rq[i].vq, &vi->rq[i].napi);
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 <makita.toshiaki@lab.ntt.co.jp> --- drivers/net/virtio_net.c | 32 +++++++++++++++++++++++++------- 1 file changed, 25 insertions(+), 7 deletions(-)