* [PATCH bpf 0/2] xsk: fix non-existing flag/option behavior @ 2019-03-08 7:57 Björn Töpel 2019-03-08 7:57 ` [PATCH bpf 1/2] xsk: fix to reject invalid flags in xsk_bind Björn Töpel 2019-03-08 7:57 ` [PATCH bpf 2/2] xsk: fix to reject invalid options in Tx descriptor Björn Töpel 0 siblings, 2 replies; 8+ messages in thread From: Björn Töpel @ 2019-03-08 7:57 UTC (permalink / raw) To: ast, daniel, netdev Cc: Björn Töpel, magnus.karlsson, magnus.karlsson, bpf 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH bpf 1/2] xsk: fix to reject invalid flags in xsk_bind 2019-03-08 7:57 [PATCH bpf 0/2] xsk: fix non-existing flag/option behavior Björn Töpel @ 2019-03-08 7:57 ` Björn Töpel 2019-03-08 9:59 ` Maciej Fijalkowski 2019-03-08 7:57 ` [PATCH bpf 2/2] xsk: fix to reject invalid options in Tx descriptor Björn Töpel 1 sibling, 1 reply; 8+ messages in thread From: Björn Töpel @ 2019-03-08 7:57 UTC (permalink / raw) To: ast, daniel, netdev Cc: Björn Töpel, magnus.karlsson, magnus.karlsson, bpf 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH bpf 1/2] xsk: fix to reject invalid flags in xsk_bind 2019-03-08 7:57 ` [PATCH bpf 1/2] xsk: fix to reject invalid flags in xsk_bind Björn Töpel @ 2019-03-08 9:59 ` Maciej Fijalkowski 2019-03-08 10:11 ` Björn Töpel 0 siblings, 1 reply; 8+ messages in thread From: Maciej Fijalkowski @ 2019-03-08 9:59 UTC (permalink / raw) To: Björn Töpel Cc: ast, daniel, netdev, Björn Töpel, magnus.karlsson, magnus.karlsson, bpf 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; ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH bpf 1/2] xsk: fix to reject invalid flags in xsk_bind 2019-03-08 9:59 ` Maciej Fijalkowski @ 2019-03-08 10:11 ` Björn Töpel 2019-03-08 10:46 ` Maciej Fijalkowski 0 siblings, 1 reply; 8+ messages in thread From: Björn Töpel @ 2019-03-08 10:11 UTC (permalink / raw) To: Maciej Fijalkowski Cc: Alexei Starovoitov, Daniel Borkmann, Netdev, Björn Töpel, Karlsson, Magnus, Magnus Karlsson, bpf 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; > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH bpf 1/2] xsk: fix to reject invalid flags in xsk_bind 2019-03-08 10:11 ` Björn Töpel @ 2019-03-08 10:46 ` Maciej Fijalkowski 2019-03-08 11:06 ` Björn Töpel 0 siblings, 1 reply; 8+ messages in thread From: Maciej Fijalkowski @ 2019-03-08 10:46 UTC (permalink / raw) To: Björn Töpel Cc: Alexei Starovoitov, Daniel Borkmann, Netdev, Björn Töpel, Karlsson, Magnus, Magnus Karlsson, bpf 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; > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH bpf 1/2] xsk: fix to reject invalid flags in xsk_bind 2019-03-08 10:46 ` Maciej Fijalkowski @ 2019-03-08 11:06 ` Björn Töpel 2019-03-08 13:26 ` Daniel Borkmann 0 siblings, 1 reply; 8+ messages in thread From: Björn Töpel @ 2019-03-08 11:06 UTC (permalink / raw) To: Maciej Fijalkowski Cc: Alexei Starovoitov, Daniel Borkmann, Netdev, Björn Töpel, Karlsson, Magnus, Magnus Karlsson, bpf 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH bpf 1/2] xsk: fix to reject invalid flags in xsk_bind 2019-03-08 11:06 ` Björn Töpel @ 2019-03-08 13:26 ` Daniel Borkmann 0 siblings, 0 replies; 8+ messages in thread From: Daniel Borkmann @ 2019-03-08 13:26 UTC (permalink / raw) To: Björn Töpel, Maciej Fijalkowski Cc: Alexei Starovoitov, Netdev, Björn Töpel, Karlsson, Magnus, Magnus Karlsson, bpf 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH bpf 2/2] xsk: fix to reject invalid options in Tx descriptor 2019-03-08 7:57 [PATCH bpf 0/2] xsk: fix non-existing flag/option behavior Björn Töpel 2019-03-08 7:57 ` [PATCH bpf 1/2] xsk: fix to reject invalid flags in xsk_bind Björn Töpel @ 2019-03-08 7:57 ` Björn Töpel 1 sibling, 0 replies; 8+ messages in thread From: Björn Töpel @ 2019-03-08 7:57 UTC (permalink / raw) To: ast, daniel, netdev Cc: Björn Töpel, magnus.karlsson, magnus.karlsson, bpf 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-03-08 13:26 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-03-08 7:57 [PATCH bpf 0/2] xsk: fix non-existing flag/option behavior Björn Töpel 2019-03-08 7:57 ` [PATCH bpf 1/2] xsk: fix to reject invalid flags in xsk_bind Björn Töpel 2019-03-08 9:59 ` Maciej Fijalkowski 2019-03-08 10:11 ` Björn Töpel 2019-03-08 10:46 ` Maciej Fijalkowski 2019-03-08 11:06 ` Björn Töpel 2019-03-08 13:26 ` Daniel Borkmann 2019-03-08 7:57 ` [PATCH bpf 2/2] xsk: fix to reject invalid options in Tx descriptor Björn Töpel
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.