| Message ID | 20191205155028.28854-2-maximmi@mellanox.com |
|---|---|
| State | Awaiting Upstream |
| Headers | show |
| Series | Fix concurrency issues between XSK wakeup and control path using RCU | expand |
On Thu, 5 Dec 2019 at 16:52, Maxim Mikityanskiy <maximmi@mellanox.com> wrote: > > The XSK wakeup callback in drivers makes some sanity checks before > triggering NAPI. However, some configuration changes may occur during > this function that affect the result of those checks. For example, the > interface can go down, and all the resources will be destroyed after the > checks in the wakeup function, but before it attempts to use these > resources. Wrap this callback in rcu_read_lock to allow driver to > synchronize_rcu before actually destroying the resources. > Thanks for taking a deeper look! > Signed-off-by: Maxim Mikityanskiy <maximmi@mellanox.com> > --- > net/xdp/xsk.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c > index 956793893c9d..d2261c90f03a 100644 > --- a/net/xdp/xsk.c > +++ b/net/xdp/xsk.c > @@ -337,9 +337,13 @@ EXPORT_SYMBOL(xsk_umem_consume_tx); > static int xsk_zc_xmit(struct xdp_sock *xs) > { > struct net_device *dev = xs->dev; > + int err; > > - return dev->netdev_ops->ndo_xsk_wakeup(dev, xs->queue_id, > - XDP_WAKEUP_TX); > + rcu_read_lock(); > + err = dev->netdev_ops->ndo_xsk_wakeup(dev, xs->queue_id, XDP_WAKEUP_TX); > + rcu_read_unlock(); > + The rationale for the the not having any synchronization on the ndo_xsk_wakeup was to not constrain the drivers. The idea was to let drivers take care of the required synchronization themselves, since this is most likely driver specific. I'd prefer leaving that to the driver implementors, not having the read-lock in the generic AF_XDP code. (And note that the ndo_xsk_wakeup is also called in the poll() implementation.) I don't think this is needed for the Intel drivers, but let me elaborate on that in those patches. Note "think" here -- I might be way off here! :-) > + return err; > } > > static void xsk_destruct_skb(struct sk_buff *skb) > -- > 2.20.1 >
diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c index 956793893c9d..d2261c90f03a 100644 --- a/net/xdp/xsk.c +++ b/net/xdp/xsk.c @@ -337,9 +337,13 @@ EXPORT_SYMBOL(xsk_umem_consume_tx); static int xsk_zc_xmit(struct xdp_sock *xs) { struct net_device *dev = xs->dev; + int err; - return dev->netdev_ops->ndo_xsk_wakeup(dev, xs->queue_id, - XDP_WAKEUP_TX); + rcu_read_lock(); + err = dev->netdev_ops->ndo_xsk_wakeup(dev, xs->queue_id, XDP_WAKEUP_TX); + rcu_read_unlock(); + + return err; } static void xsk_destruct_skb(struct sk_buff *skb)
The XSK wakeup callback in drivers makes some sanity checks before triggering NAPI. However, some configuration changes may occur during this function that affect the result of those checks. For example, the interface can go down, and all the resources will be destroyed after the checks in the wakeup function, but before it attempts to use these resources. Wrap this callback in rcu_read_lock to allow driver to synchronize_rcu before actually destroying the resources. Signed-off-by: Maxim Mikityanskiy <maximmi@mellanox.com> --- net/xdp/xsk.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)