[PATCH bpf 0/2] xsk: fix non-existing flag/option behavior

[PATCH bpf 0/2] xsk: fix non-existing flag/option behavior

[Björn Töpel]

These two patches addresses the behavior of xsk_bind and Tx descriptor
validation, when non-existing flags are set.


Thanks,
Björn


Björn Töpel (2):
  xsk: fix to reject invalid flags in xsk_bind
  xsk: fix to reject invalid options in Tx descriptor

 net/xdp/xsk.c       | 5 ++++-
 net/xdp/xsk_queue.h | 4 ++--
 2 files changed, 6 insertions(+), 3 deletions(-)

-- 
2.19.1

[PATCH bpf 1/2] xsk: fix to reject invalid flags in xsk_bind

[Björn Töpel]

From: Björn Töpel <bjorn.topel@intel.com>

Passing a non-existing flag in the sxdp_flags member of struct
sockaddr_xdp was, incorrectly, silently ignored. This patch addresses
that behavior, and rejects any non-existing flags.

We have examined existing user space code, and to our best knowledge,
no one is relying on the current incorrect behavior. AF_XDP is still
in its infancy, so from our perspective, the risk of breakage is very
low, and addressing this problem now is important.

Fixes: 965a99098443 ("xsk: add support for bind for Rx")
Signed-off-by: Björn Töpel <bjorn.topel@intel.com>
---
 net/xdp/xsk.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
index 6697084e3fdf..a14e8864e4fa 100644
--- a/net/xdp/xsk.c
+++ b/net/xdp/xsk.c
@@ -407,6 +407,10 @@ static int xsk_bind(struct socket *sock, struct sockaddr *addr, int addr_len)
 	if (sxdp->sxdp_family != AF_XDP)
 		return -EINVAL;
 
+	flags = sxdp->sxdp_flags;
+	if (flags & ~(XDP_SHARED_UMEM | XDP_COPY | XDP_ZEROCOPY))
+		return -EINVAL;
+
 	mutex_lock(&xs->mutex);
 	if (xs->dev) {
 		err = -EBUSY;
@@ -425,7 +429,6 @@ static int xsk_bind(struct socket *sock, struct sockaddr *addr, int addr_len)
 	}
 
 	qid = sxdp->sxdp_queue_id;
-	flags = sxdp->sxdp_flags;
 
 	if (flags & XDP_SHARED_UMEM) {
 		struct xdp_sock *umem_xs;
-- 
2.19.1

Re: [PATCH bpf 1/2] xsk: fix to reject invalid flags in xsk_bind

[Maciej Fijalkowski]

On Fri,  8 Mar 2019 08:57:26 +0100
Björn Töpel <bjorn.topel@gmail.com> wrote:

> From: Björn Töpel <bjorn.topel@intel.com>
> 
> Passing a non-existing flag in the sxdp_flags member of struct
> sockaddr_xdp was, incorrectly, silently ignored. This patch addresses
> that behavior, and rejects any non-existing flags.
> 
> We have examined existing user space code, and to our best knowledge,
> no one is relying on the current incorrect behavior. AF_XDP is still
> in its infancy, so from our perspective, the risk of breakage is very
> low, and addressing this problem now is important.
> 
> Fixes: 965a99098443 ("xsk: add support for bind for Rx")
> Signed-off-by: Björn Töpel <bjorn.topel@intel.com>
> ---
>  net/xdp/xsk.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> index 6697084e3fdf..a14e8864e4fa 100644
> --- a/net/xdp/xsk.c
> +++ b/net/xdp/xsk.c
> @@ -407,6 +407,10 @@ static int xsk_bind(struct socket *sock, struct sockaddr *addr, int addr_len)
>  	if (sxdp->sxdp_family != AF_XDP)
>  		return -EINVAL;
>  
> +	flags = sxdp->sxdp_flags;
> +	if (flags & ~(XDP_SHARED_UMEM | XDP_COPY | XDP_ZEROCOPY))
> +		return -EINVAL;
> +

What about setting more than one flag at a time? Is it allowed/make any sense?
After a quick look it seems that they exclude each other, e.g. you can't force
a zero copy and copy mode at the same time. And for XDP_SHARED_UMEM two
remaining flags can't be set.

So maybe check here also that only one particular flag is set by doing:

if (hweight32(flags & (XDP_SHARED_UMEM | XDP_COPY | XDP_ZEROCOPY)) > 1)
	return -EINVAL;

just like we do it for IFLA_XDP_FLAGS in net/core/rtnetlink.c?

>  	mutex_lock(&xs->mutex);
>  	if (xs->dev) {
>  		err = -EBUSY;
> @@ -425,7 +429,6 @@ static int xsk_bind(struct socket *sock, struct sockaddr *addr, int addr_len)
>  	}
>  
>  	qid = sxdp->sxdp_queue_id;
> -	flags = sxdp->sxdp_flags;
>  
>  	if (flags & XDP_SHARED_UMEM) {
>  		struct xdp_sock *umem_xs;

Re: [PATCH bpf 1/2] xsk: fix to reject invalid flags in xsk_bind

[Björn Töpel]

On Fri, 8 Mar 2019 at 11:00, Maciej Fijalkowski
<maciejromanfijalkowski@gmail.com> wrote:
>
> On Fri,  8 Mar 2019 08:57:26 +0100
> Björn Töpel <bjorn.topel@gmail.com> wrote:
>
> > From: Björn Töpel <bjorn.topel@intel.com>
> >
> > Passing a non-existing flag in the sxdp_flags member of struct
> > sockaddr_xdp was, incorrectly, silently ignored. This patch addresses
> > that behavior, and rejects any non-existing flags.
> >
> > We have examined existing user space code, and to our best knowledge,
> > no one is relying on the current incorrect behavior. AF_XDP is still
> > in its infancy, so from our perspective, the risk of breakage is very
> > low, and addressing this problem now is important.
> >
> > Fixes: 965a99098443 ("xsk: add support for bind for Rx")
> > Signed-off-by: Björn Töpel <bjorn.topel@intel.com>
> > ---
> >  net/xdp/xsk.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> > index 6697084e3fdf..a14e8864e4fa 100644
> > --- a/net/xdp/xsk.c
> > +++ b/net/xdp/xsk.c
> > @@ -407,6 +407,10 @@ static int xsk_bind(struct socket *sock, struct sockaddr *addr, int addr_len)
> >       if (sxdp->sxdp_family != AF_XDP)
> >               return -EINVAL;
> >
> > +     flags = sxdp->sxdp_flags;
> > +     if (flags & ~(XDP_SHARED_UMEM | XDP_COPY | XDP_ZEROCOPY))
> > +             return -EINVAL;
> > +
>
> What about setting more than one flag at a time? Is it allowed/make any sense?
> After a quick look it seems that they exclude each other, e.g. you can't force
> a zero copy and copy mode at the same time. And for XDP_SHARED_UMEM two
> remaining flags can't be set.
>
> So maybe check here also that only one particular flag is set by doing:
>
> if (hweight32(flags & (XDP_SHARED_UMEM | XDP_COPY | XDP_ZEROCOPY)) > 1)
>         return -EINVAL;
>
> just like we do it for IFLA_XDP_FLAGS in net/core/rtnetlink.c?
>

We have flag semantic checks further down, and my rational was to
*only* check unknown flags first. IMO the current patch is easier to
understand, than your suggested one.

Thanks for taking a look!

Cheers,
Björn

> >       mutex_lock(&xs->mutex);
> >       if (xs->dev) {
> >               err = -EBUSY;
> > @@ -425,7 +429,6 @@ static int xsk_bind(struct socket *sock, struct sockaddr *addr, int addr_len)
> >       }
> >
> >       qid = sxdp->sxdp_queue_id;
> > -     flags = sxdp->sxdp_flags;
> >
> >       if (flags & XDP_SHARED_UMEM) {
> >               struct xdp_sock *umem_xs;
>

Re: [PATCH bpf 1/2] xsk: fix to reject invalid flags in xsk_bind

[Maciej Fijalkowski]

On Fri, 8 Mar 2019 11:11:05 +0100
Björn Töpel <bjorn.topel@gmail.com> wrote:

> On Fri, 8 Mar 2019 at 11:00, Maciej Fijalkowski
> <maciejromanfijalkowski@gmail.com> wrote:
> >
> > On Fri,  8 Mar 2019 08:57:26 +0100
> > Björn Töpel <bjorn.topel@gmail.com> wrote:
> >  
> > > From: Björn Töpel <bjorn.topel@intel.com>
> > >
> > > Passing a non-existing flag in the sxdp_flags member of struct
> > > sockaddr_xdp was, incorrectly, silently ignored. This patch addresses
> > > that behavior, and rejects any non-existing flags.
> > >
> > > We have examined existing user space code, and to our best knowledge,
> > > no one is relying on the current incorrect behavior. AF_XDP is still
> > > in its infancy, so from our perspective, the risk of breakage is very
> > > low, and addressing this problem now is important.
> > >
> > > Fixes: 965a99098443 ("xsk: add support for bind for Rx")
> > > Signed-off-by: Björn Töpel <bjorn.topel@intel.com>
> > > ---
> > >  net/xdp/xsk.c | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> > > index 6697084e3fdf..a14e8864e4fa 100644
> > > --- a/net/xdp/xsk.c
> > > +++ b/net/xdp/xsk.c
> > > @@ -407,6 +407,10 @@ static int xsk_bind(struct socket *sock, struct sockaddr *addr, int addr_len)
> > >       if (sxdp->sxdp_family != AF_XDP)
> > >               return -EINVAL;
> > >
> > > +     flags = sxdp->sxdp_flags;
> > > +     if (flags & ~(XDP_SHARED_UMEM | XDP_COPY | XDP_ZEROCOPY))
> > > +             return -EINVAL;
> > > +  
> >
> > What about setting more than one flag at a time? Is it allowed/make any sense?
> > After a quick look it seems that they exclude each other, e.g. you can't force
> > a zero copy and copy mode at the same time. And for XDP_SHARED_UMEM two
> > remaining flags can't be set.
> >
> > So maybe check here also that only one particular flag is set by doing:
> >
> > if (hweight32(flags & (XDP_SHARED_UMEM | XDP_COPY | XDP_ZEROCOPY)) > 1)
> >         return -EINVAL;
> >
> > just like we do it for IFLA_XDP_FLAGS in net/core/rtnetlink.c?
> >  
> 
> We have flag semantic checks further down, and my rational was to
> *only* check unknown flags first. IMO the current patch is easier to
> understand, than your suggested one.
> 

Hmm thought that bailing out earlier would be better and we could drop the
actual copy flags checks for shared umem. For xdp_umem_assign_dev() instead of
passing flags you could just pass a boolean whether you're doing zero copy or
not. And that brings up the question whether we really need a XDP_COPY flag?

> Thanks for taking a look!
> 
> Cheers,
> Björn
> 
> > >       mutex_lock(&xs->mutex);
> > >       if (xs->dev) {
> > >               err = -EBUSY;
> > > @@ -425,7 +429,6 @@ static int xsk_bind(struct socket *sock, struct sockaddr *addr, int addr_len)
> > >       }
> > >
> > >       qid = sxdp->sxdp_queue_id;
> > > -     flags = sxdp->sxdp_flags;
> > >
> > >       if (flags & XDP_SHARED_UMEM) {
> > >               struct xdp_sock *umem_xs;  
> >  

Re: [PATCH bpf 1/2] xsk: fix to reject invalid flags in xsk_bind

[Björn Töpel]

On Fri, 8 Mar 2019 at 11:46, Maciej Fijalkowski
<maciejromanfijalkowski@gmail.com> wrote:
>
[...]
> > > > +     flags = sxdp->sxdp_flags;
> > > > +     if (flags & ~(XDP_SHARED_UMEM | XDP_COPY | XDP_ZEROCOPY))
> > > > +             return -EINVAL;
> > > > +
> > >
> > > What about setting more than one flag at a time? Is it allowed/make any sense?
> > > After a quick look it seems that they exclude each other, e.g. you can't force
> > > a zero copy and copy mode at the same time. And for XDP_SHARED_UMEM two
> > > remaining flags can't be set.
> > >
> > > So maybe check here also that only one particular flag is set by doing:
> > >
> > > if (hweight32(flags & (XDP_SHARED_UMEM | XDP_COPY | XDP_ZEROCOPY)) > 1)
> > >         return -EINVAL;
> > >
> > > just like we do it for IFLA_XDP_FLAGS in net/core/rtnetlink.c?
> > >
> >
> > We have flag semantic checks further down, and my rational was to
> > *only* check unknown flags first. IMO the current patch is easier to
> > understand, than your suggested one.
> >
>
> Hmm thought that bailing out earlier would be better and we could drop the
> actual copy flags checks for shared umem. For xdp_umem_assign_dev() instead of
> passing flags you could just pass a boolean whether you're doing zero copy or
> not. And that brings up the question whether we really need a XDP_COPY flag?
>

I'd prefer doing that as a follow-up patch.

XDP_COPY is needed to explicitly enable copy-mode. No flags is "select
the best option", and COPY/ZEROCOPY is to explicitly select a mode.

Re: [PATCH bpf 1/2] xsk: fix to reject invalid flags in xsk_bind

[Daniel Borkmann]

On 03/08/2019 12:06 PM, Björn Töpel wrote:
> On Fri, 8 Mar 2019 at 11:46, Maciej Fijalkowski
> <maciejromanfijalkowski@gmail.com> wrote:
[...]
>>>> So maybe check here also that only one particular flag is set by doing:
>>>>
>>>> if (hweight32(flags & (XDP_SHARED_UMEM | XDP_COPY | XDP_ZEROCOPY)) > 1)
>>>>         return -EINVAL;
>>>>
>>>> just like we do it for IFLA_XDP_FLAGS in net/core/rtnetlink.c?
>>>
>>> We have flag semantic checks further down, and my rational was to
>>> *only* check unknown flags first. IMO the current patch is easier to
>>> understand, than your suggested one.
>>
>> Hmm thought that bailing out earlier would be better and we could drop the
>> actual copy flags checks for shared umem. For xdp_umem_assign_dev() instead of
>> passing flags you could just pass a boolean whether you're doing zero copy or
>> not. And that brings up the question whether we really need a XDP_COPY flag?
> 
> I'd prefer doing that as a follow-up patch.

Ok, follow-up for Maciej's suggestion is fine, imho, it would definitely make
it more obvious resp. future proof such that rejecting some invalid combination
doesn't get missed under some branch. Given it's currently not the case aside
from the fix, rolling this into your next patches for bpf-next might be best.
In any case, fixes look good, so applied, thanks!

> XDP_COPY is needed to explicitly enable copy-mode. No flags is "select
> the best option", and COPY/ZEROCOPY is to explicitly select a mode.

[PATCH bpf 2/2] xsk: fix to reject invalid options in Tx descriptor

[Björn Töpel]

From: Björn Töpel <bjorn.topel@intel.com>

Passing a non-existing option in the options member of struct
xdp_desc, incorrectly, silently ignored. This patch addresses that
behavior, and drops any Tx descriptor with non-existing options.

We have examined existing user space code, and to our best knowledge,
no one is relying on the current incorrect behavior. AF_XDP is still
in its infancy, so from our perspective, the risk of breakage is very
low, and addressing this problem now is important.

Fixes: 35fcde7f8deb ("xsk: support for Tx")
Signed-off-by: Björn Töpel <bjorn.topel@intel.com>
---
 net/xdp/xsk_queue.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/xdp/xsk_queue.h b/net/xdp/xsk_queue.h
index bcb5cbb40419..610c0bdc0c2b 100644
--- a/net/xdp/xsk_queue.h
+++ b/net/xdp/xsk_queue.h
@@ -174,8 +174,8 @@ static inline bool xskq_is_valid_desc(struct xsk_queue *q, struct xdp_desc *d)
 	if (!xskq_is_valid_addr(q, d->addr))
 		return false;
 
-	if (((d->addr + d->len) & q->chunk_mask) !=
-	    (d->addr & q->chunk_mask)) {
+	if (((d->addr + d->len) & q->chunk_mask) != (d->addr & q->chunk_mask) ||
+	    d->options) {
 		q->invalid_descs++;
 		return false;
 	}
-- 
2.19.1