search for: virtnet_open

When initializing XDP in virtnet_open(), some rq xdp initialization may hit an error causing net device open failed. However, previous rqs have already initialized XDP and enabled NAPI, which is not the expected behavior. Need to roll back the previous rq initialization to avoid leaks in error unwinding of init code. Also extract help...

...5 p.m.10:33, Xuan Zhuo wrote: >>>>> External email: Use caution opening links or attachments >>>>> >>>>> >>>>> On Tue, 2 May 2023 20:35:25 -0400, Feng Liu <feliu at nvidia.com> wrote: >>>>>> When initializing XDP in virtnet_open(), some rq xdp initialization >>>>>> may hit an error causing net device open failed. However, previous >>>>>> rqs have already initialized XDP and enabled NAPI, which is not the >>>>>> expected behavior. Need to roll back the previous rq initia...

...t; On 2023-05-05 p.m.10:33, Xuan Zhuo wrote: > >>> External email: Use caution opening links or attachments > >>> > >>> > >>> On Tue, 2 May 2023 20:35:25 -0400, Feng Liu <feliu at nvidia.com> wrote: > >>>> When initializing XDP in virtnet_open(), some rq xdp initialization > >>>> may hit an error causing net device open failed. However, previous > >>>> rqs have already initialized XDP and enabled NAPI, which is not the > >>>> expected behavior. Need to roll back the previous rq initialization...

...1 at 09:12:38PM +1030, Rusty Russell wrote: > Michael S. Tsirkin noticed that we could run the refill work after > ndo_close, which can re-enable napi - we don't disable it until > virtnet_remove. This is clearly wrong, so move the workqueue control > to ndo_open and ndo_stop (aka. virtnet_open and virtnet_close). > > One subtle point: virtnet_probe() could simply fail if it couldn't > allocate a receive buffer, but that's less polite in virtnet_open() so > we schedule a refill as we do in the normal receive path if we run out > of memory. > > Signed-off-by:...

...1 at 09:12:38PM +1030, Rusty Russell wrote: > Michael S. Tsirkin noticed that we could run the refill work after > ndo_close, which can re-enable napi - we don't disable it until > virtnet_remove. This is clearly wrong, so move the workqueue control > to ndo_open and ndo_stop (aka. virtnet_open and virtnet_close). > > One subtle point: virtnet_probe() could simply fail if it couldn't > allocate a receive buffer, but that's less polite in virtnet_open() so > we schedule a refill as we do in the normal receive path if we run out > of memory. > > Signed-off-by:...

When initializing XDP in virtnet_open(), some rq xdp initialization may hit an error causing net device open failed. However, previous rqs have already initialized XDP and enabled NAPI, which is not the expected behavior. Need to roll back the previous rq initialization to avoid leaks in error unwinding of init code. Fixes: 754b8a21a9...

When initializing XDP in virtnet_open(), some rq xdp initialization may hit an error causing net device open failed. However, previous rqs have already initialized XDP and enabled NAPI, which is not the expected behavior. Need to roll back the previous rq initialization to avoid leaks in error unwinding of init code. Also extract a he...

On Tue, May 02, 2023 at 01:41:34PM -0400, Feng Liu wrote: > When initializing XDP in virtnet_open(), some rq xdp initialization > may hit an error causing net device open failed. However, previous > rqs have already initialized XDP and enabled NAPI, which is not the > expected behavior. Need to roll back the previous rq initialization > to avoid leaks in error unwinding of init code...

On Tue, 2 May 2023 20:35:25 -0400, Feng Liu <feliu at nvidia.com> wrote: > When initializing XDP in virtnet_open(), some rq xdp initialization > may hit an error causing net device open failed. However, previous > rqs have already initialized XDP and enabled NAPI, which is not the > expected behavior. Need to roll back the previous rq initialization > to avoid leaks in error unwinding of init code...

When initializing XDP in virtnet_open(), some rq xdp initialization may hit an error causing net device open failed. However, previous rqs have already initialized XDP and enabled NAPI, which is not the expected behavior. Need to roll back the previous rq initialization to avoid leaks in error unwinding of init code. Also extract a he...

On 2023-05-05 p.m.10:33, Xuan Zhuo wrote: > External email: Use caution opening links or attachments > > > On Tue, 2 May 2023 20:35:25 -0400, Feng Liu <feliu at nvidia.com> wrote: >> When initializing XDP in virtnet_open(), some rq xdp initialization >> may hit an error causing net device open failed. However, previous >> rqs have already initialized XDP and enabled NAPI, which is not the >> expected behavior. Need to roll back the previous rq initialization >> to avoid leaks in error unwind...

...he refill process looks quite inefficient to me, it not buggy. napi_disable() is a big hammer, while whole idea of having a process to block on GFP_KERNEL allocations is to allow some asynchronous behavior. I have hard time to convince myself virtio_net is safe anyway with this work queue thing. virtnet_open() seems racy for example : for (i = 0; i < vi->max_queue_pairs; i++) { if (i < vi->curr_queue_pairs) /* Make sure we have some buffers: if oom use wq. */ if (!try_fill_recv(&vi->rq[i], GFP_KERNEL))...

...he refill process looks quite inefficient to me, it not buggy. napi_disable() is a big hammer, while whole idea of having a process to block on GFP_KERNEL allocations is to allow some asynchronous behavior. I have hard time to convince myself virtio_net is safe anyway with this work queue thing. virtnet_open() seems racy for example : for (i = 0; i < vi->max_queue_pairs; i++) { if (i < vi->curr_queue_pairs) /* Make sure we have some buffers: if oom use wq. */ if (!try_fill_recv(&vi->rq[i], GFP_KERNEL))...

...ange the RX-ring queue layout and backing pages, but instead reject XDP setup if all the conditions for XDP is not meet. Thus, the xdp_rxq_info also remains fairly static. This allow us to simply add the reg/unreg to net_device open/close functions. Driver hook points for xdp_rxq_info: * reg : virtnet_open * unreg: virtnet_close V3: - bugfix, also setup xdp.rxq in receive_mergeable() - Tested bpf-sample prog inside guest on a virtio_net device Cc: "Michael S. Tsirkin" <mst at redhat.com> Cc: Jason Wang <jasowang at redhat.com> Cc: virtualization at lists.linux-foundation.or...

...struct net_device *dev, u32 queue_index) +{ + int err; + + err = xdp_rxq_info_reg(xdp_rxq, dev, queue_index); + if (err < 0) + return err; + + err = xdp_rxq_info_reg_mem_model(xdp_rxq, MEM_TYPE_PAGE_SHARED, NULL); + if (err < 0) + xdp_rxq_info_unreg(xdp_rxq); + return err; +} + static int virtnet_open(struct net_device *dev) { struct virtnet_info *vi = netdev_priv(dev); @@ -1480,17 +1495,10 @@ static int virtnet_open(struct net_device *dev) if (!try_fill_recv(vi, &vi->rq[i], GFP_KERNEL)) schedule_delayed_work(&vi->refill, 0); - err = xdp_rxq_info_reg(&vi->rq[i...

...struct net_device *dev, u32 queue_index) +{ + int err; + + err = xdp_rxq_info_reg(xdp_rxq, dev, queue_index); + if (err < 0) + return err; + + err = xdp_rxq_info_reg_mem_model(xdp_rxq, MEM_TYPE_PAGE_SHARED, NULL); + if (err < 0) + xdp_rxq_info_unreg(xdp_rxq); + return err; +} + static int virtnet_open(struct net_device *dev) { struct virtnet_info *vi = netdev_priv(dev); @@ -1480,17 +1495,10 @@ static int virtnet_open(struct net_device *dev) if (!try_fill_recv(vi, &vi->rq[i], GFP_KERNEL)) schedule_delayed_work(&vi->refill, 0); - err = xdp_rxq_info_reg(&vi->rq[i...

...ange the RX-ring queue layout and backing pages, but instead reject XDP setup if all the conditions for XDP is not meet. Thus, the xdp_rxq_info also remains fairly static. This allow us to simply add the reg/unreg to net_device open/close functions. Driver hook points for xdp_rxq_info: * reg : virtnet_open * unreg: virtnet_close Cc: "Michael S. Tsirkin" <mst at redhat.com> Cc: Jason Wang <jasowang at redhat.com> Cc: virtualization at lists.linux-foundation.org Signed-off-by: Jesper Dangaard Brouer <brouer at redhat.com> --- drivers/net/virtio_net.c | 12 +++++++++++-...

...but not sufficient. There is also the issue that refill_work() could be scheduled. If refill_work() executes, it will re-enable NAPI. We'd need to cancel the vi->refill delayed work to prevent this AFAICT, and also ensure that no other function re-schedules vi->refill or re-enables NAPI (virtnet_open/close, virtnet_set_queues, and virtnet_freeze/restore). How is the following sequence of operations: rtnl_lock(); cancel_delayed_work_sync(&vi->refill); napi_disable(&rq->napi); read rq->mrg_avg_pkt_len virtnet_enable_napi(); rtnl_unlock(); Additionally, if we disable NAPI when r...

...but not sufficient. There is also the issue that refill_work() could be scheduled. If refill_work() executes, it will re-enable NAPI. We'd need to cancel the vi->refill delayed work to prevent this AFAICT, and also ensure that no other function re-schedules vi->refill or re-enables NAPI (virtnet_open/close, virtnet_set_queues, and virtnet_freeze/restore). How is the following sequence of operations: rtnl_lock(); cancel_delayed_work_sync(&vi->refill); napi_disable(&rq->napi); read rq->mrg_avg_pkt_len virtnet_enable_napi(); rtnl_unlock(); Additionally, if we disable NAPI when r...

...me any running queue is timed out. doesn't look right. also - there's already a warning in this case in the core. do we need another one? > + dev->stats.tx_errors++; > + } > +} > + > static const struct net_device_ops virtnet_netdev = { > .ndo_open = virtnet_open, > .ndo_stop = virtnet_close, > @@ -2600,6 +2623,7 @@ static const struct net_device_ops virtnet_netdev = { > .ndo_features_check = passthru_features_check, > .ndo_get_phys_port_name = virtnet_get_phys_port_name, > .ndo_set_features = virtnet_set_features, > + .ndo...