* [PATCH net 0/2] Fix collisions in socket cookie generation @ 2019-08-08 9:49 Daniel Borkmann 2019-08-08 9:49 ` [PATCH net 1/2] sock: make cookie generation global instead of per netns Daniel Borkmann 2019-08-08 9:49 ` [PATCH net 2/2] bpf: sync bpf.h to tools infrastructure Daniel Borkmann 0 siblings, 2 replies; 7+ messages in thread From: Daniel Borkmann @ 2019-08-08 9:49 UTC (permalink / raw) To: davem; +Cc: netdev, bpf, m, edumazet, ast, willemb, Daniel Borkmann This change makes the socket cookie generator as a global counter instead of per netns in order to fix cookie collisions for BPF use cases we ran into. See main patch #1 for more details. Given the change is small/trivial and fixes an issue we're seeing my preference would be net tree (though it cleanly applies to net-next as well). Went for net tree instead of bpf tree here given the main change is in net/core/sock_diag.c, but either way would be fine with me. Thanks a lot! Daniel Borkmann (2): sock: make cookie generation global instead of per netns bpf: sync bpf.h to tools infrastructure include/net/net_namespace.h | 1 - include/uapi/linux/bpf.h | 4 ++-- net/core/sock_diag.c | 3 ++- tools/include/uapi/linux/bpf.h | 11 +++++++---- 4 files changed, 11 insertions(+), 8 deletions(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH net 1/2] sock: make cookie generation global instead of per netns 2019-08-08 9:49 [PATCH net 0/2] Fix collisions in socket cookie generation Daniel Borkmann @ 2019-08-08 9:49 ` Daniel Borkmann 2019-08-08 10:45 ` Eric Dumazet 2019-08-08 9:49 ` [PATCH net 2/2] bpf: sync bpf.h to tools infrastructure Daniel Borkmann 1 sibling, 1 reply; 7+ messages in thread From: Daniel Borkmann @ 2019-08-08 9:49 UTC (permalink / raw) To: davem; +Cc: netdev, bpf, m, edumazet, ast, willemb, Daniel Borkmann Generating and retrieving socket cookies are a useful feature that is exposed to BPF for various program types through bpf_get_socket_cookie() helper. The fact that the cookie counter is per netns is quite a limitation for BPF in practice in particular for programs in host namespace that use socket cookies as part of a map lookup key since they will be causing socket cookie collisions e.g. when attached to BPF cgroup hooks or cls_bpf on tc egress in host namespace handling container traffic from veths or ipvlan slaves. Change the counter to be global instead. Socket cookie consumers must assume the value as opqaue in any case. The cookie does not guarantee an always unique identifier since it could wrap in fabricated corner cases where two sockets could end up holding the same cookie, but is good enough to be used as a hint for many use cases; not every socket must have a cookie generated hence knowledge of the counter value does not provide much value either way. Signed-off-by: Daniel Borkmann <daniel@iogearbox.net> Cc: Eric Dumazet <edumazet@google.com> Cc: Alexei Starovoitov <ast@kernel.org> Cc: Willem de Bruijn <willemb@google.com> Cc: Martynas Pumputis <m@lambda.lt> --- include/net/net_namespace.h | 1 - include/uapi/linux/bpf.h | 4 ++-- net/core/sock_diag.c | 3 ++- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/include/net/net_namespace.h b/include/net/net_namespace.h index 4a9da951a794..cb668bc2692d 100644 --- a/include/net/net_namespace.h +++ b/include/net/net_namespace.h @@ -61,7 +61,6 @@ struct net { spinlock_t rules_mod_lock; u32 hash_mix; - atomic64_t cookie_gen; struct list_head list; /* list of network namespaces */ struct list_head exit_list; /* To linked to call pernet exit diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h index fa1c753dcdbc..a5aa7d3ac6a1 100644 --- a/include/uapi/linux/bpf.h +++ b/include/uapi/linux/bpf.h @@ -1466,8 +1466,8 @@ union bpf_attr { * If no cookie has been set yet, generate a new cookie. Once * generated, the socket cookie remains stable for the life of the * socket. This helper can be useful for monitoring per socket - * networking traffic statistics as it provides a unique socket - * identifier per namespace. + * networking traffic statistics as it provides a global socket + * identifier that can be assumed unique. * Return * A 8-byte long non-decreasing number on success, or 0 if the * socket field is missing inside *skb*. diff --git a/net/core/sock_diag.c b/net/core/sock_diag.c index 3312a5849a97..c13ffbd33d8d 100644 --- a/net/core/sock_diag.c +++ b/net/core/sock_diag.c @@ -19,6 +19,7 @@ static const struct sock_diag_handler *sock_diag_handlers[AF_MAX]; static int (*inet_rcv_compat)(struct sk_buff *skb, struct nlmsghdr *nlh); static DEFINE_MUTEX(sock_diag_table_mutex); static struct workqueue_struct *broadcast_wq; +static atomic64_t cookie_gen; u64 sock_gen_cookie(struct sock *sk) { @@ -27,7 +28,7 @@ u64 sock_gen_cookie(struct sock *sk) if (res) return res; - res = atomic64_inc_return(&sock_net(sk)->cookie_gen); + res = atomic64_inc_return(&cookie_gen); atomic64_cmpxchg(&sk->sk_cookie, 0, res); } } -- 2.17.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH net 1/2] sock: make cookie generation global instead of per netns 2019-08-08 9:49 ` [PATCH net 1/2] sock: make cookie generation global instead of per netns Daniel Borkmann @ 2019-08-08 10:45 ` Eric Dumazet 2019-08-08 11:09 ` Daniel Borkmann 0 siblings, 1 reply; 7+ messages in thread From: Eric Dumazet @ 2019-08-08 10:45 UTC (permalink / raw) To: Daniel Borkmann Cc: David Miller, netdev, bpf, m, Alexei Starovoitov, Willem de Bruijn On Thu, Aug 8, 2019 at 11:50 AM Daniel Borkmann <daniel@iogearbox.net> wrote: > > Socket cookie consumers must assume the value as opqaue in any case. > The cookie does not guarantee an always unique identifier since it > could wrap in fabricated corner cases where two sockets could end up > holding the same cookie, What do you mean by this ? Cookie is guaranteed to be unique, it is from a 64bit counter... There should be no collision. > but is good enough to be used as a hint for > many use cases; not every socket must have a cookie generated hence > knowledge of the counter value does not provide much value either way. > > Signed-off-by: Daniel Borkmann <daniel@iogearbox.net> > Cc: Eric Dumazet <edumazet@google.com> > Cc: Alexei Starovoitov <ast@kernel.org> > Cc: Willem de Bruijn <willemb@google.com> > Cc: Martynas Pumputis <m@lambda.lt> > --- > include/net/net_namespace.h | 1 - > include/uapi/linux/bpf.h | 4 ++-- > net/core/sock_diag.c | 3 ++- > 3 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/include/net/net_namespace.h b/include/net/net_namespace.h > index 4a9da951a794..cb668bc2692d 100644 > --- a/include/net/net_namespace.h > +++ b/include/net/net_namespace.h > @@ -61,7 +61,6 @@ struct net { > spinlock_t rules_mod_lock; > > u32 hash_mix; > - atomic64_t cookie_gen; > > struct list_head list; /* list of network namespaces */ > struct list_head exit_list; /* To linked to call pernet exit > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h > index fa1c753dcdbc..a5aa7d3ac6a1 100644 > --- a/include/uapi/linux/bpf.h > +++ b/include/uapi/linux/bpf.h > @@ -1466,8 +1466,8 @@ union bpf_attr { > * If no cookie has been set yet, generate a new cookie. Once > * generated, the socket cookie remains stable for the life of the > * socket. This helper can be useful for monitoring per socket > - * networking traffic statistics as it provides a unique socket > - * identifier per namespace. > + * networking traffic statistics as it provides a global socket > + * identifier that can be assumed unique. > * Return > * A 8-byte long non-decreasing number on success, or 0 if the > * socket field is missing inside *skb*. > diff --git a/net/core/sock_diag.c b/net/core/sock_diag.c > index 3312a5849a97..c13ffbd33d8d 100644 > --- a/net/core/sock_diag.c > +++ b/net/core/sock_diag.c > @@ -19,6 +19,7 @@ static const struct sock_diag_handler *sock_diag_handlers[AF_MAX]; > static int (*inet_rcv_compat)(struct sk_buff *skb, struct nlmsghdr *nlh); > static DEFINE_MUTEX(sock_diag_table_mutex); > static struct workqueue_struct *broadcast_wq; > +static atomic64_t cookie_gen; > > u64 sock_gen_cookie(struct sock *sk) > { > @@ -27,7 +28,7 @@ u64 sock_gen_cookie(struct sock *sk) > > if (res) > return res; > - res = atomic64_inc_return(&sock_net(sk)->cookie_gen); > + res = atomic64_inc_return(&cookie_gen); > atomic64_cmpxchg(&sk->sk_cookie, 0, res); > } > } > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net 1/2] sock: make cookie generation global instead of per netns 2019-08-08 10:45 ` Eric Dumazet @ 2019-08-08 11:09 ` Daniel Borkmann 2019-08-08 11:37 ` Eric Dumazet 0 siblings, 1 reply; 7+ messages in thread From: Daniel Borkmann @ 2019-08-08 11:09 UTC (permalink / raw) To: Eric Dumazet Cc: David Miller, netdev, bpf, m, Alexei Starovoitov, Willem de Bruijn On 8/8/19 12:45 PM, Eric Dumazet wrote: > On Thu, Aug 8, 2019 at 11:50 AM Daniel Borkmann <daniel@iogearbox.net> wrote: > >> Socket cookie consumers must assume the value as opqaue in any case. >> The cookie does not guarantee an always unique identifier since it >> could wrap in fabricated corner cases where two sockets could end up >> holding the same cookie, > > What do you mean by this ? > > Cookie is guaranteed to be unique, it is from a 64bit counter... > > There should be no collision. I meant the [theoretical] corner case where socket_1 has cookie X and we'd create, trigger sock_gen_cookie() to increment, close socket in a loop until we wrap and get another cookie X for socket_2; agree it's impractical and for little gain anyway. So in practice there should be no collision which is what I tried to say. Thanks, Daniel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net 1/2] sock: make cookie generation global instead of per netns 2019-08-08 11:09 ` Daniel Borkmann @ 2019-08-08 11:37 ` Eric Dumazet 2019-08-08 11:40 ` Daniel Borkmann 0 siblings, 1 reply; 7+ messages in thread From: Eric Dumazet @ 2019-08-08 11:37 UTC (permalink / raw) To: Daniel Borkmann Cc: David Miller, netdev, bpf, m, Alexei Starovoitov, Willem de Bruijn On Thu, Aug 8, 2019 at 1:09 PM Daniel Borkmann <daniel@iogearbox.net> wrote: > > On 8/8/19 12:45 PM, Eric Dumazet wrote: > > On Thu, Aug 8, 2019 at 11:50 AM Daniel Borkmann <daniel@iogearbox.net> wrote: > > > >> Socket cookie consumers must assume the value as opqaue in any case. > >> The cookie does not guarantee an always unique identifier since it > >> could wrap in fabricated corner cases where two sockets could end up > >> holding the same cookie, > > > > What do you mean by this ? > > > > Cookie is guaranteed to be unique, it is from a 64bit counter... > > > > There should be no collision. > > I meant the [theoretical] corner case where socket_1 has cookie X and > we'd create, trigger sock_gen_cookie() to increment, close socket in a > loop until we wrap and get another cookie X for socket_2; agree it's > impractical and for little gain anyway. So in practice there should be > no collision which is what I tried to say. If a 64bit counter, updated by one unit at a time could overflow during the lifetime of a host, I would agree with you, but this can not happen, even if we succeed to make 1 billion locked increments per second (this would still need 584 years) I would prefer not mentioning something that can not possibly happen in your changelog ;) ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net 1/2] sock: make cookie generation global instead of per netns 2019-08-08 11:37 ` Eric Dumazet @ 2019-08-08 11:40 ` Daniel Borkmann 0 siblings, 0 replies; 7+ messages in thread From: Daniel Borkmann @ 2019-08-08 11:40 UTC (permalink / raw) To: Eric Dumazet Cc: David Miller, netdev, bpf, m, Alexei Starovoitov, Willem de Bruijn On 8/8/19 1:37 PM, Eric Dumazet wrote: > On Thu, Aug 8, 2019 at 1:09 PM Daniel Borkmann <daniel@iogearbox.net> wrote: >> On 8/8/19 12:45 PM, Eric Dumazet wrote: >>> On Thu, Aug 8, 2019 at 11:50 AM Daniel Borkmann <daniel@iogearbox.net> wrote: >>> >>>> Socket cookie consumers must assume the value as opqaue in any case. >>>> The cookie does not guarantee an always unique identifier since it >>>> could wrap in fabricated corner cases where two sockets could end up >>>> holding the same cookie, >>> >>> What do you mean by this ? >>> >>> Cookie is guaranteed to be unique, it is from a 64bit counter... >>> >>> There should be no collision. >> >> I meant the [theoretical] corner case where socket_1 has cookie X and >> we'd create, trigger sock_gen_cookie() to increment, close socket in a >> loop until we wrap and get another cookie X for socket_2; agree it's >> impractical and for little gain anyway. So in practice there should be >> no collision which is what I tried to say. > > If a 64bit counter, updated by one unit at a time could overflow > during the lifetime of a host, > I would agree with you, but this can not happen, even if we succeed to > make 1 billion > locked increments per second (this would still need 584 years) > > I would prefer not mentioning something that can not possibly happen > in your changelog ;) Yep fair enough, makes sense. I'll fix it :) ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH net 2/2] bpf: sync bpf.h to tools infrastructure 2019-08-08 9:49 [PATCH net 0/2] Fix collisions in socket cookie generation Daniel Borkmann 2019-08-08 9:49 ` [PATCH net 1/2] sock: make cookie generation global instead of per netns Daniel Borkmann @ 2019-08-08 9:49 ` Daniel Borkmann 1 sibling, 0 replies; 7+ messages in thread From: Daniel Borkmann @ 2019-08-08 9:49 UTC (permalink / raw) To: davem; +Cc: netdev, bpf, m, edumazet, ast, willemb, Daniel Borkmann Pull in updates in BPF helper function description. Signed-off-by: Daniel Borkmann <daniel@iogearbox.net> --- tools/include/uapi/linux/bpf.h | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h index 4e455018da65..a5aa7d3ac6a1 100644 --- a/tools/include/uapi/linux/bpf.h +++ b/tools/include/uapi/linux/bpf.h @@ -1466,8 +1466,8 @@ union bpf_attr { * If no cookie has been set yet, generate a new cookie. Once * generated, the socket cookie remains stable for the life of the * socket. This helper can be useful for monitoring per socket - * networking traffic statistics as it provides a unique socket - * identifier per namespace. + * networking traffic statistics as it provides a global socket + * identifier that can be assumed unique. * Return * A 8-byte long non-decreasing number on success, or 0 if the * socket field is missing inside *skb*. @@ -1571,8 +1571,11 @@ union bpf_attr { * but this is only implemented for native XDP (with driver * support) as of this writing). * - * All values for *flags* are reserved for future usage, and must - * be left at zero. + * The lower two bits of *flags* are used as the return code if + * the map lookup fails. This is so that the return value can be + * one of the XDP program return codes up to XDP_TX, as chosen by + * the caller. Any higher bits in the *flags* argument must be + * unset. * * When used to redirect packets to net devices, this helper * provides a high performance increase over **bpf_redirect**\ (). -- 2.17.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-08-08 11:40 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-08-08 9:49 [PATCH net 0/2] Fix collisions in socket cookie generation Daniel Borkmann 2019-08-08 9:49 ` [PATCH net 1/2] sock: make cookie generation global instead of per netns Daniel Borkmann 2019-08-08 10:45 ` Eric Dumazet 2019-08-08 11:09 ` Daniel Borkmann 2019-08-08 11:37 ` Eric Dumazet 2019-08-08 11:40 ` Daniel Borkmann 2019-08-08 9:49 ` [PATCH net 2/2] bpf: sync bpf.h to tools infrastructure Daniel Borkmann
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).