[bpf-next] xsk: proper socket state check in xsk_poll
diff mbox series

Message ID 20190820100405.25564-1-bjorn.topel@gmail.com
State Changes Requested
Delegated to: BPF Maintainers
Headers show
Series
  • [bpf-next] xsk: proper socket state check in xsk_poll
Related show

Commit Message

Björn Töpel Aug. 20, 2019, 10:04 a.m. UTC
From: Björn Töpel <bjorn.topel@intel.com>

The poll() implementation for AF_XDP sockets did not perform the
proper state checks, prior accessing the socket umem. This patch fixes
that by performing a xsk_is_bound() check.

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 | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

Comments

Daniel Borkmann Aug. 20, 2019, 2:30 p.m. UTC | #1
On 8/20/19 12:04 PM, Björn Töpel wrote:
> From: Björn Töpel <bjorn.topel@intel.com>
> 
> The poll() implementation for AF_XDP sockets did not perform the
> proper state checks, prior accessing the socket umem. This patch fixes
> that by performing a xsk_is_bound() check.
> 
> 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 | 14 ++++++++++++--
>   1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> index ee4428a892fa..08bed5e92af4 100644
> --- a/net/xdp/xsk.c
> +++ b/net/xdp/xsk.c
> @@ -356,13 +356,20 @@ static int xsk_generic_xmit(struct sock *sk, struct msghdr *m,
>   	return err;
>   }
>   
> +static bool xsk_is_bound(struct xdp_sock *xs)
> +{
> +	struct net_device *dev = READ_ONCE(xs->dev);
> +
> +	return dev && xs->state == XSK_BOUND;
> +}
> +
>   static int xsk_sendmsg(struct socket *sock, struct msghdr *m, size_t total_len)
>   {
>   	bool need_wait = !(m->msg_flags & MSG_DONTWAIT);
>   	struct sock *sk = sock->sk;
>   	struct xdp_sock *xs = xdp_sk(sk);
>   
> -	if (unlikely(!xs->dev))
> +	if (unlikely(!xsk_is_bound(xs)))
>   		return -ENXIO;
>   	if (unlikely(!(xs->dev->flags & IFF_UP)))
>   		return -ENETDOWN;
> @@ -383,6 +390,9 @@ static unsigned int xsk_poll(struct file *file, struct socket *sock,
>   	struct net_device *dev = xs->dev;
>   	struct xdp_umem *umem = xs->umem;
>   
> +	if (unlikely(!xsk_is_bound(xs)))
> +		return mask;
> +
>   	if (umem->need_wakeup)
>   		dev->netdev_ops->ndo_xsk_wakeup(dev, xs->queue_id,
>   						umem->need_wakeup);
> @@ -417,7 +427,7 @@ static void xsk_unbind_dev(struct xdp_sock *xs)
>   {
>   	struct net_device *dev = xs->dev;
>   
> -	if (!dev || xs->state != XSK_BOUND)
> +	if (!xsk_is_bound(xs))
>   		return;

I think I'm a bit confused by your READ_ONCE() usage. ;-/ I can see why you're
using it in xsk_is_bound() above, but then at the same time all the other callbacks
like xsk_poll() or xsk_unbind_dev() above have a struct net_device *dev = xs->dev
right before the test. Could you elaborate?

Thanks,
Daniel
Björn Töpel Aug. 20, 2019, 3:29 p.m. UTC | #2
On Tue, 20 Aug 2019 at 16:30, Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 8/20/19 12:04 PM, Björn Töpel wrote:
> > From: Björn Töpel <bjorn.topel@intel.com>
> >
> > The poll() implementation for AF_XDP sockets did not perform the
> > proper state checks, prior accessing the socket umem. This patch fixes
> > that by performing a xsk_is_bound() check.
> >
> > 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 | 14 ++++++++++++--
> >   1 file changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> > index ee4428a892fa..08bed5e92af4 100644
> > --- a/net/xdp/xsk.c
> > +++ b/net/xdp/xsk.c
> > @@ -356,13 +356,20 @@ static int xsk_generic_xmit(struct sock *sk, struct msghdr *m,
> >       return err;
> >   }
> >
> > +static bool xsk_is_bound(struct xdp_sock *xs)
> > +{
> > +     struct net_device *dev = READ_ONCE(xs->dev);
> > +
> > +     return dev && xs->state == XSK_BOUND;
> > +}
> > +
> >   static int xsk_sendmsg(struct socket *sock, struct msghdr *m, size_t total_len)
> >   {
> >       bool need_wait = !(m->msg_flags & MSG_DONTWAIT);
> >       struct sock *sk = sock->sk;
> >       struct xdp_sock *xs = xdp_sk(sk);
> >
> > -     if (unlikely(!xs->dev))
> > +     if (unlikely(!xsk_is_bound(xs)))
> >               return -ENXIO;
> >       if (unlikely(!(xs->dev->flags & IFF_UP)))
> >               return -ENETDOWN;
> > @@ -383,6 +390,9 @@ static unsigned int xsk_poll(struct file *file, struct socket *sock,
> >       struct net_device *dev = xs->dev;
> >       struct xdp_umem *umem = xs->umem;
> >
> > +     if (unlikely(!xsk_is_bound(xs)))
> > +             return mask;
> > +
> >       if (umem->need_wakeup)
> >               dev->netdev_ops->ndo_xsk_wakeup(dev, xs->queue_id,
> >                                               umem->need_wakeup);
> > @@ -417,7 +427,7 @@ static void xsk_unbind_dev(struct xdp_sock *xs)
> >   {
> >       struct net_device *dev = xs->dev;
> >
> > -     if (!dev || xs->state != XSK_BOUND)
> > +     if (!xsk_is_bound(xs))
> >               return;
>
> I think I'm a bit confused by your READ_ONCE() usage. ;-/ I can see why you're
> using it in xsk_is_bound() above, but then at the same time all the other callbacks
> like xsk_poll() or xsk_unbind_dev() above have a struct net_device *dev = xs->dev
> right before the test. Could you elaborate?
>

Yes, now I'm confused as well! Digging deeper... I believe there are a
couple of places in xsk.c that do not have
READ_ONCE/WRITE_ONCE-correctness. Various xdp_sock members are read
lock-less outside the control plane mutex (mutex member of struct
xdp_sock). This needs some re-work. I'll look into using the newly
introduced state member (with corresponding read/write barriers) for
this.

I'll cook some patch(es) that address this, but first it sounds like I
need to reread [1] two, or three times. At least. ;-)


Thanks,
Björn


[1] https://lwn.net/Articles/793253/


> Thanks,
> Daniel
Daniel Borkmann Aug. 20, 2019, 9:24 p.m. UTC | #3
On 8/20/19 5:29 PM, Björn Töpel wrote:
> On Tue, 20 Aug 2019 at 16:30, Daniel Borkmann <daniel@iogearbox.net> wrote:
>> On 8/20/19 12:04 PM, Björn Töpel wrote:
>>> From: Björn Töpel <bjorn.topel@intel.com>
>>>
>>> The poll() implementation for AF_XDP sockets did not perform the
>>> proper state checks, prior accessing the socket umem. This patch fixes
>>> that by performing a xsk_is_bound() check.
>>>
>>> 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 | 14 ++++++++++++--
>>>    1 file changed, 12 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
>>> index ee4428a892fa..08bed5e92af4 100644
>>> --- a/net/xdp/xsk.c
>>> +++ b/net/xdp/xsk.c
>>> @@ -356,13 +356,20 @@ static int xsk_generic_xmit(struct sock *sk, struct msghdr *m,
>>>        return err;
>>>    }
>>>
>>> +static bool xsk_is_bound(struct xdp_sock *xs)
>>> +{
>>> +     struct net_device *dev = READ_ONCE(xs->dev);
>>> +
>>> +     return dev && xs->state == XSK_BOUND;
>>> +}
>>> +
>>>    static int xsk_sendmsg(struct socket *sock, struct msghdr *m, size_t total_len)
>>>    {
>>>        bool need_wait = !(m->msg_flags & MSG_DONTWAIT);
>>>        struct sock *sk = sock->sk;
>>>        struct xdp_sock *xs = xdp_sk(sk);
>>>
>>> -     if (unlikely(!xs->dev))
>>> +     if (unlikely(!xsk_is_bound(xs)))
>>>                return -ENXIO;
>>>        if (unlikely(!(xs->dev->flags & IFF_UP)))
>>>                return -ENETDOWN;
>>> @@ -383,6 +390,9 @@ static unsigned int xsk_poll(struct file *file, struct socket *sock,
>>>        struct net_device *dev = xs->dev;
>>>        struct xdp_umem *umem = xs->umem;
>>>
>>> +     if (unlikely(!xsk_is_bound(xs)))
>>> +             return mask;
>>> +
>>>        if (umem->need_wakeup)
>>>                dev->netdev_ops->ndo_xsk_wakeup(dev, xs->queue_id,
>>>                                                umem->need_wakeup);
>>> @@ -417,7 +427,7 @@ static void xsk_unbind_dev(struct xdp_sock *xs)
>>>    {
>>>        struct net_device *dev = xs->dev;
>>>
>>> -     if (!dev || xs->state != XSK_BOUND)
>>> +     if (!xsk_is_bound(xs))
>>>                return;
>>
>> I think I'm a bit confused by your READ_ONCE() usage. ;-/ I can see why you're
>> using it in xsk_is_bound() above, but then at the same time all the other callbacks
>> like xsk_poll() or xsk_unbind_dev() above have a struct net_device *dev = xs->dev
>> right before the test. Could you elaborate?
> 
> Yes, now I'm confused as well! Digging deeper... I believe there are a
> couple of places in xsk.c that do not have
> READ_ONCE/WRITE_ONCE-correctness. Various xdp_sock members are read
> lock-less outside the control plane mutex (mutex member of struct
> xdp_sock). This needs some re-work. I'll look into using the newly

Right, so even in above two cases, the compiler could have refetched, e.g.
dev variable could have first been NULL, but xsk_is_bound() later returns
true.

> introduced state member (with corresponding read/write barriers) for
> this.
> 
> I'll cook some patch(es) that address this, but first it sounds like I
> need to reread [1] two, or three times. At least. ;-)
> 
> 
> Thanks,
> Björn
> 
> 
> [1] https://lwn.net/Articles/793253/
> 
> 
>> Thanks,
>> Daniel

Patch
diff mbox series

diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
index ee4428a892fa..08bed5e92af4 100644
--- a/net/xdp/xsk.c
+++ b/net/xdp/xsk.c
@@ -356,13 +356,20 @@  static int xsk_generic_xmit(struct sock *sk, struct msghdr *m,
 	return err;
 }
 
+static bool xsk_is_bound(struct xdp_sock *xs)
+{
+	struct net_device *dev = READ_ONCE(xs->dev);
+
+	return dev && xs->state == XSK_BOUND;
+}
+
 static int xsk_sendmsg(struct socket *sock, struct msghdr *m, size_t total_len)
 {
 	bool need_wait = !(m->msg_flags & MSG_DONTWAIT);
 	struct sock *sk = sock->sk;
 	struct xdp_sock *xs = xdp_sk(sk);
 
-	if (unlikely(!xs->dev))
+	if (unlikely(!xsk_is_bound(xs)))
 		return -ENXIO;
 	if (unlikely(!(xs->dev->flags & IFF_UP)))
 		return -ENETDOWN;
@@ -383,6 +390,9 @@  static unsigned int xsk_poll(struct file *file, struct socket *sock,
 	struct net_device *dev = xs->dev;
 	struct xdp_umem *umem = xs->umem;
 
+	if (unlikely(!xsk_is_bound(xs)))
+		return mask;
+
 	if (umem->need_wakeup)
 		dev->netdev_ops->ndo_xsk_wakeup(dev, xs->queue_id,
 						umem->need_wakeup);
@@ -417,7 +427,7 @@  static void xsk_unbind_dev(struct xdp_sock *xs)
 {
 	struct net_device *dev = xs->dev;
 
-	if (!dev || xs->state != XSK_BOUND)
+	if (!xsk_is_bound(xs))
 		return;
 
 	xs->state = XSK_UNBOUND;