Message ID | 20190822091306.20581-3-bjorn.topel@gmail.com |
---|---|
State | Changes Requested |
Delegated to: | BPF Maintainers |
Headers | show |
Series | xsk: various CPU barrier and {READ, WRITE}_ONCE fixes | expand |
On 22 Aug 2019, at 2:13, Björn Töpel wrote: > From: Björn Töpel <bjorn.topel@intel.com> > > The state variable was read, and written outside the control mutex > (struct xdp_sock, mutex), without proper barriers and {READ, > WRITE}_ONCE correctness. > > In this commit this issue is addressed, and the state member is now > used a point of synchronization whether the socket is setup correctly > or not. > > This also fixes a race, found by syzcaller, in xsk_poll() where umem > could be accessed when stale. > > Suggested-by: Hillf Danton <hdanton@sina.com> > Reported-by: syzbot+c82697e3043781e08802@syzkaller.appspotmail.com > Fixes: 77cd0d7b3f25 ("xsk: add support for need_wakeup flag in AF_XDP > rings") > Signed-off-by: Björn Töpel <bjorn.topel@intel.com> > --- > net/xdp/xsk.c | 57 > ++++++++++++++++++++++++++++++++++++--------------- > 1 file changed, 41 insertions(+), 16 deletions(-) > > diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c > index f3351013c2a5..31236e61069b 100644 > --- a/net/xdp/xsk.c > +++ b/net/xdp/xsk.c > @@ -162,10 +162,23 @@ static int __xsk_rcv_zc(struct xdp_sock *xs, > struct xdp_buff *xdp, u32 len) > return err; > } > > +static bool xsk_is_bound(struct xdp_sock *xs) > +{ > + if (READ_ONCE(xs->state) == XSK_BOUND) { > + /* Matches smp_wmb() in bind(). */ > + smp_rmb(); > + return true; > + } > + return false; > +} > + > int xsk_rcv(struct xdp_sock *xs, struct xdp_buff *xdp) > { > u32 len; > > + if (!xsk_is_bound(xs)) > + return -EINVAL; > + > if (xs->dev != xdp->rxq->dev || xs->queue_id != > xdp->rxq->queue_index) > return -EINVAL; > > @@ -362,6 +375,8 @@ static int xsk_sendmsg(struct socket *sock, struct > msghdr *m, size_t total_len) > struct sock *sk = sock->sk; > struct xdp_sock *xs = xdp_sk(sk); > > + if (unlikely(!xsk_is_bound(xs))) > + return -ENXIO; > if (unlikely(!xs->dev)) > return -ENXIO; Can probably remove the xs->dev check now, replaced by checking xs->state, right? > if (unlikely(!(xs->dev->flags & IFF_UP))) > @@ -378,10 +393,15 @@ static unsigned int xsk_poll(struct file *file, > struct socket *sock, > struct poll_table_struct *wait) > { > unsigned int mask = datagram_poll(file, sock, wait); > - struct sock *sk = sock->sk; > - struct xdp_sock *xs = xdp_sk(sk); > - struct net_device *dev = xs->dev; > - struct xdp_umem *umem = xs->umem; > + struct xdp_sock *xs = xdp_sk(sock->sk); > + struct net_device *dev; > + struct xdp_umem *umem; > + > + if (unlikely(!xsk_is_bound(xs))) > + return mask; > + > + dev = xs->dev; > + umem = xs->umem; > > if (umem->need_wakeup) > dev->netdev_ops->ndo_xsk_wakeup(dev, xs->queue_id, > @@ -417,10 +437,9 @@ static void xsk_unbind_dev(struct xdp_sock *xs) > { > struct net_device *dev = xs->dev; > > - if (!dev || xs->state != XSK_BOUND) > + if (xs->state != XSK_BOUND) > return; > - > - xs->state = XSK_UNBOUND; > + WRITE_ONCE(xs->state, XSK_UNBOUND); > > /* Wait for driver to stop using the xdp socket. */ > xdp_del_sk_umem(xs->umem, xs); > @@ -495,7 +514,9 @@ static int xsk_release(struct socket *sock) > local_bh_enable(); > > xsk_delete_from_maps(xs); > + mutex_lock(&xs->mutex); > xsk_unbind_dev(xs); > + mutex_unlock(&xs->mutex); > > xskq_destroy(xs->rx); > xskq_destroy(xs->tx); > @@ -589,19 +610,18 @@ static int xsk_bind(struct socket *sock, struct > sockaddr *addr, int addr_len) > } > > umem_xs = xdp_sk(sock->sk); > - if (!umem_xs->umem) { > - /* No umem to inherit. */ > + if (!xsk_is_bound(umem_xs)) { > err = -EBADF; > sockfd_put(sock); > goto out_unlock; > - } else if (umem_xs->dev != dev || umem_xs->queue_id != qid) { > + } > + if (umem_xs->dev != dev || umem_xs->queue_id != qid) { > err = -EINVAL; > sockfd_put(sock); > goto out_unlock; > } > - > xdp_get_umem(umem_xs->umem); > - xs->umem = umem_xs->umem; > + WRITE_ONCE(xs->umem, umem_xs->umem); > sockfd_put(sock); > } else if (!xs->umem || !xdp_umem_validate_queues(xs->umem)) { > err = -EINVAL; > @@ -626,10 +646,15 @@ static int xsk_bind(struct socket *sock, struct > sockaddr *addr, int addr_len) > xdp_add_sk_umem(xs->umem, xs); > > out_unlock: > - if (err) > + if (err) { > dev_put(dev); > - else > - xs->state = XSK_BOUND; > + } else { > + /* Matches smp_rmb() in bind() for shared umem > + * sockets, and xsk_is_bound(). > + */ > + smp_wmb(); > + WRITE_ONCE(xs->state, XSK_BOUND); > + } > out_release: > mutex_unlock(&xs->mutex); > rtnl_unlock(); > @@ -869,7 +894,7 @@ static int xsk_mmap(struct file *file, struct > socket *sock, > unsigned long pfn; > struct page *qpg; > > - if (xs->state != XSK_READY) > + if (READ_ONCE(xs->state) != XSK_READY) > return -EBUSY; > > if (offset == XDP_PGOFF_RX_RING) { > -- > 2.20.1
On Fri, 23 Aug 2019 at 18:46, Jonathan Lemon <jonathan.lemon@gmail.com> wrote: > > > > On 22 Aug 2019, at 2:13, Björn Töpel wrote: > > > From: Björn Töpel <bjorn.topel@intel.com> > > > > The state variable was read, and written outside the control mutex > > (struct xdp_sock, mutex), without proper barriers and {READ, > > WRITE}_ONCE correctness. > > > > In this commit this issue is addressed, and the state member is now > > used a point of synchronization whether the socket is setup correctly > > or not. > > > > This also fixes a race, found by syzcaller, in xsk_poll() where umem > > could be accessed when stale. > > > > Suggested-by: Hillf Danton <hdanton@sina.com> > > Reported-by: syzbot+c82697e3043781e08802@syzkaller.appspotmail.com > > Fixes: 77cd0d7b3f25 ("xsk: add support for need_wakeup flag in AF_XDP > > rings") > > Signed-off-by: Björn Töpel <bjorn.topel@intel.com> > > --- > > net/xdp/xsk.c | 57 > > ++++++++++++++++++++++++++++++++++++--------------- > > 1 file changed, 41 insertions(+), 16 deletions(-) > > > > diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c > > index f3351013c2a5..31236e61069b 100644 > > --- a/net/xdp/xsk.c > > +++ b/net/xdp/xsk.c > > @@ -162,10 +162,23 @@ static int __xsk_rcv_zc(struct xdp_sock *xs, > > struct xdp_buff *xdp, u32 len) > > return err; > > } > > > > +static bool xsk_is_bound(struct xdp_sock *xs) > > +{ > > + if (READ_ONCE(xs->state) == XSK_BOUND) { > > + /* Matches smp_wmb() in bind(). */ > > + smp_rmb(); > > + return true; > > + } > > + return false; > > +} > > + > > int xsk_rcv(struct xdp_sock *xs, struct xdp_buff *xdp) > > { > > u32 len; > > > > + if (!xsk_is_bound(xs)) > > + return -EINVAL; > > + > > if (xs->dev != xdp->rxq->dev || xs->queue_id != > > xdp->rxq->queue_index) > > return -EINVAL; > > > > @@ -362,6 +375,8 @@ static int xsk_sendmsg(struct socket *sock, struct > > msghdr *m, size_t total_len) > > struct sock *sk = sock->sk; > > struct xdp_sock *xs = xdp_sk(sk); > > > > + if (unlikely(!xsk_is_bound(xs))) > > + return -ENXIO; > > if (unlikely(!xs->dev)) > > return -ENXIO; > > Can probably remove the xs->dev check now, replaced by checking > xs->state, right? > Indeed! Thanks for catching; I'll send out a v2. Björn > > > if (unlikely(!(xs->dev->flags & IFF_UP))) > > @@ -378,10 +393,15 @@ static unsigned int xsk_poll(struct file *file, > > struct socket *sock, > > struct poll_table_struct *wait) > > { > > unsigned int mask = datagram_poll(file, sock, wait); > > - struct sock *sk = sock->sk; > > - struct xdp_sock *xs = xdp_sk(sk); > > - struct net_device *dev = xs->dev; > > - struct xdp_umem *umem = xs->umem; > > + struct xdp_sock *xs = xdp_sk(sock->sk); > > + struct net_device *dev; > > + struct xdp_umem *umem; > > + > > + if (unlikely(!xsk_is_bound(xs))) > > + return mask; > > + > > + dev = xs->dev; > > + umem = xs->umem; > > > > if (umem->need_wakeup) > > dev->netdev_ops->ndo_xsk_wakeup(dev, xs->queue_id, > > @@ -417,10 +437,9 @@ static void xsk_unbind_dev(struct xdp_sock *xs) > > { > > struct net_device *dev = xs->dev; > > > > - if (!dev || xs->state != XSK_BOUND) > > + if (xs->state != XSK_BOUND) > > return; > > - > > - xs->state = XSK_UNBOUND; > > + WRITE_ONCE(xs->state, XSK_UNBOUND); > > > > /* Wait for driver to stop using the xdp socket. */ > > xdp_del_sk_umem(xs->umem, xs); > > @@ -495,7 +514,9 @@ static int xsk_release(struct socket *sock) > > local_bh_enable(); > > > > xsk_delete_from_maps(xs); > > + mutex_lock(&xs->mutex); > > xsk_unbind_dev(xs); > > + mutex_unlock(&xs->mutex); > > > > xskq_destroy(xs->rx); > > xskq_destroy(xs->tx); > > @@ -589,19 +610,18 @@ static int xsk_bind(struct socket *sock, struct > > sockaddr *addr, int addr_len) > > } > > > > umem_xs = xdp_sk(sock->sk); > > - if (!umem_xs->umem) { > > - /* No umem to inherit. */ > > + if (!xsk_is_bound(umem_xs)) { > > err = -EBADF; > > sockfd_put(sock); > > goto out_unlock; > > - } else if (umem_xs->dev != dev || umem_xs->queue_id != qid) { > > + } > > + if (umem_xs->dev != dev || umem_xs->queue_id != qid) { > > err = -EINVAL; > > sockfd_put(sock); > > goto out_unlock; > > } > > - > > xdp_get_umem(umem_xs->umem); > > - xs->umem = umem_xs->umem; > > + WRITE_ONCE(xs->umem, umem_xs->umem); > > sockfd_put(sock); > > } else if (!xs->umem || !xdp_umem_validate_queues(xs->umem)) { > > err = -EINVAL; > > @@ -626,10 +646,15 @@ static int xsk_bind(struct socket *sock, struct > > sockaddr *addr, int addr_len) > > xdp_add_sk_umem(xs->umem, xs); > > > > out_unlock: > > - if (err) > > + if (err) { > > dev_put(dev); > > - else > > - xs->state = XSK_BOUND; > > + } else { > > + /* Matches smp_rmb() in bind() for shared umem > > + * sockets, and xsk_is_bound(). > > + */ > > + smp_wmb(); > > + WRITE_ONCE(xs->state, XSK_BOUND); > > + } > > out_release: > > mutex_unlock(&xs->mutex); > > rtnl_unlock(); > > @@ -869,7 +894,7 @@ static int xsk_mmap(struct file *file, struct > > socket *sock, > > unsigned long pfn; > > struct page *qpg; > > > > - if (xs->state != XSK_READY) > > + if (READ_ONCE(xs->state) != XSK_READY) > > return -EBUSY; > > > > if (offset == XDP_PGOFF_RX_RING) { > > -- > > 2.20.1
diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c index f3351013c2a5..31236e61069b 100644 --- a/net/xdp/xsk.c +++ b/net/xdp/xsk.c @@ -162,10 +162,23 @@ static int __xsk_rcv_zc(struct xdp_sock *xs, struct xdp_buff *xdp, u32 len) return err; } +static bool xsk_is_bound(struct xdp_sock *xs) +{ + if (READ_ONCE(xs->state) == XSK_BOUND) { + /* Matches smp_wmb() in bind(). */ + smp_rmb(); + return true; + } + return false; +} + int xsk_rcv(struct xdp_sock *xs, struct xdp_buff *xdp) { u32 len; + if (!xsk_is_bound(xs)) + return -EINVAL; + if (xs->dev != xdp->rxq->dev || xs->queue_id != xdp->rxq->queue_index) return -EINVAL; @@ -362,6 +375,8 @@ static int xsk_sendmsg(struct socket *sock, struct msghdr *m, size_t total_len) struct sock *sk = sock->sk; struct xdp_sock *xs = xdp_sk(sk); + if (unlikely(!xsk_is_bound(xs))) + return -ENXIO; if (unlikely(!xs->dev)) return -ENXIO; if (unlikely(!(xs->dev->flags & IFF_UP))) @@ -378,10 +393,15 @@ static unsigned int xsk_poll(struct file *file, struct socket *sock, struct poll_table_struct *wait) { unsigned int mask = datagram_poll(file, sock, wait); - struct sock *sk = sock->sk; - struct xdp_sock *xs = xdp_sk(sk); - struct net_device *dev = xs->dev; - struct xdp_umem *umem = xs->umem; + struct xdp_sock *xs = xdp_sk(sock->sk); + struct net_device *dev; + struct xdp_umem *umem; + + if (unlikely(!xsk_is_bound(xs))) + return mask; + + dev = xs->dev; + umem = xs->umem; if (umem->need_wakeup) dev->netdev_ops->ndo_xsk_wakeup(dev, xs->queue_id, @@ -417,10 +437,9 @@ static void xsk_unbind_dev(struct xdp_sock *xs) { struct net_device *dev = xs->dev; - if (!dev || xs->state != XSK_BOUND) + if (xs->state != XSK_BOUND) return; - - xs->state = XSK_UNBOUND; + WRITE_ONCE(xs->state, XSK_UNBOUND); /* Wait for driver to stop using the xdp socket. */ xdp_del_sk_umem(xs->umem, xs); @@ -495,7 +514,9 @@ static int xsk_release(struct socket *sock) local_bh_enable(); xsk_delete_from_maps(xs); + mutex_lock(&xs->mutex); xsk_unbind_dev(xs); + mutex_unlock(&xs->mutex); xskq_destroy(xs->rx); xskq_destroy(xs->tx); @@ -589,19 +610,18 @@ static int xsk_bind(struct socket *sock, struct sockaddr *addr, int addr_len) } umem_xs = xdp_sk(sock->sk); - if (!umem_xs->umem) { - /* No umem to inherit. */ + if (!xsk_is_bound(umem_xs)) { err = -EBADF; sockfd_put(sock); goto out_unlock; - } else if (umem_xs->dev != dev || umem_xs->queue_id != qid) { + } + if (umem_xs->dev != dev || umem_xs->queue_id != qid) { err = -EINVAL; sockfd_put(sock); goto out_unlock; } - xdp_get_umem(umem_xs->umem); - xs->umem = umem_xs->umem; + WRITE_ONCE(xs->umem, umem_xs->umem); sockfd_put(sock); } else if (!xs->umem || !xdp_umem_validate_queues(xs->umem)) { err = -EINVAL; @@ -626,10 +646,15 @@ static int xsk_bind(struct socket *sock, struct sockaddr *addr, int addr_len) xdp_add_sk_umem(xs->umem, xs); out_unlock: - if (err) + if (err) { dev_put(dev); - else - xs->state = XSK_BOUND; + } else { + /* Matches smp_rmb() in bind() for shared umem + * sockets, and xsk_is_bound(). + */ + smp_wmb(); + WRITE_ONCE(xs->state, XSK_BOUND); + } out_release: mutex_unlock(&xs->mutex); rtnl_unlock(); @@ -869,7 +894,7 @@ static int xsk_mmap(struct file *file, struct socket *sock, unsigned long pfn; struct page *qpg; - if (xs->state != XSK_READY) + if (READ_ONCE(xs->state) != XSK_READY) return -EBUSY; if (offset == XDP_PGOFF_RX_RING) {