* Re: [PATCH bpf-next] bpf: support SO_DEBUG in bpf_setsockopt()
2019-02-03 8:15 [PATCH bpf-next] bpf: support SO_DEBUG in bpf_setsockopt() Yafang Shao
@ 2019-02-03 23:36 ` Y Song
2019-02-03 23:37 ` Lawrence Brakmo
` (2 subsequent siblings)
3 siblings, 0 replies; 8+ messages in thread
From: Y Song @ 2019-02-03 23:36 UTC (permalink / raw)
To: Yafang Shao
Cc: Martin KaFai Lau, brakmo, Alexei Starovoitov, Daniel Borkmann,
netdev, shaoyafang
On Sun, Feb 3, 2019 at 12:18 AM Yafang Shao <laoar.shao@gmail.com> wrote:
>
> Then we can enable/disable socket debugging without modifying user code.
> That is more convenient for debugging.
>
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
Acked-by: Yonghong Song <yhs@fb.com>
> ---
> include/net/sock.h | 8 ++++++++
> net/core/filter.c | 3 +++
> net/core/sock.c | 8 --------
> 3 files changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/include/net/sock.h b/include/net/sock.h
> index 2b229f7..8decee9 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -1935,6 +1935,14 @@ static inline void sock_confirm_neigh(struct sk_buff *skb, struct neighbour *n)
> }
> }
>
> +static inline void sock_valbool_flag(struct sock *sk, int bit, int valbool)
> +{
> + if (valbool)
> + sock_set_flag(sk, bit);
> + else
> + sock_reset_flag(sk, bit);
> +}
> +
> bool sk_mc_loop(struct sock *sk);
>
> static inline bool sk_can_gso(const struct sock *sk)
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 3a49f68..ce5da57 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -4111,6 +4111,9 @@ static unsigned long bpf_xdp_copy(void *dst_buff, const void *src_buff,
>
> /* Only some socketops are supported */
> switch (optname) {
> + case SO_DEBUG:
> + sock_valbool_flag(sk, SOCK_DBG, val);
> + break;
> case SO_RCVBUF:
> sk->sk_userlocks |= SOCK_RCVBUF_LOCK;
> sk->sk_rcvbuf = max_t(int, val * 2, SOCK_MIN_RCVBUF);
> diff --git a/net/core/sock.c b/net/core/sock.c
> index 900e8a9..5ef6daa 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -638,14 +638,6 @@ static int sock_getbindtodevice(struct sock *sk, char __user *optval,
> return ret;
> }
>
> -static inline void sock_valbool_flag(struct sock *sk, int bit, int valbool)
> -{
> - if (valbool)
> - sock_set_flag(sk, bit);
> - else
> - sock_reset_flag(sk, bit);
> -}
> -
> bool sk_mc_loop(struct sock *sk)
> {
> if (dev_recursion_level())
> --
> 1.8.3.1
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH bpf-next] bpf: support SO_DEBUG in bpf_setsockopt()
2019-02-03 8:15 [PATCH bpf-next] bpf: support SO_DEBUG in bpf_setsockopt() Yafang Shao
2019-02-03 23:36 ` Y Song
@ 2019-02-03 23:37 ` Lawrence Brakmo
2019-02-04 17:30 ` Quentin Monnet
2019-02-04 17:35 ` Alexei Starovoitov
3 siblings, 0 replies; 8+ messages in thread
From: Lawrence Brakmo @ 2019-02-03 23:37 UTC (permalink / raw)
To: Yafang Shao, Martin Lau, ast@kernel.org, daniel@iogearbox.net
Cc: netdev@vger.kernel.org, shaoyafang@didiglobal.com
On 2/3/19, 12:15 AM, "Yafang Shao" <laoar.shao@gmail.com> wrote:
Then we can enable/disable socket debugging without modifying user code.
That is more convenient for debugging.
Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
Acked-by: Lawrence Brakmo <brakmo@fb.com>
---
include/net/sock.h | 8 ++++++++
net/core/filter.c | 3 +++
net/core/sock.c | 8 --------
3 files changed, 11 insertions(+), 8 deletions(-)
diff --git a/include/net/sock.h b/include/net/sock.h
index 2b229f7..8decee9 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -1935,6 +1935,14 @@ static inline void sock_confirm_neigh(struct sk_buff *skb, struct neighbour *n)
}
}
+static inline void sock_valbool_flag(struct sock *sk, int bit, int valbool)
+{
+ if (valbool)
+ sock_set_flag(sk, bit);
+ else
+ sock_reset_flag(sk, bit);
+}
+
bool sk_mc_loop(struct sock *sk);
static inline bool sk_can_gso(const struct sock *sk)
diff --git a/net/core/filter.c b/net/core/filter.c
index 3a49f68..ce5da57 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -4111,6 +4111,9 @@ static unsigned long bpf_xdp_copy(void *dst_buff, const void *src_buff,
/* Only some socketops are supported */
switch (optname) {
+ case SO_DEBUG:
+ sock_valbool_flag(sk, SOCK_DBG, val);
+ break;
case SO_RCVBUF:
sk->sk_userlocks |= SOCK_RCVBUF_LOCK;
sk->sk_rcvbuf = max_t(int, val * 2, SOCK_MIN_RCVBUF);
diff --git a/net/core/sock.c b/net/core/sock.c
index 900e8a9..5ef6daa 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -638,14 +638,6 @@ static int sock_getbindtodevice(struct sock *sk, char __user *optval,
return ret;
}
-static inline void sock_valbool_flag(struct sock *sk, int bit, int valbool)
-{
- if (valbool)
- sock_set_flag(sk, bit);
- else
- sock_reset_flag(sk, bit);
-}
-
bool sk_mc_loop(struct sock *sk)
{
if (dev_recursion_level())
--
1.8.3.1
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH bpf-next] bpf: support SO_DEBUG in bpf_setsockopt()
2019-02-03 8:15 [PATCH bpf-next] bpf: support SO_DEBUG in bpf_setsockopt() Yafang Shao
2019-02-03 23:36 ` Y Song
2019-02-03 23:37 ` Lawrence Brakmo
@ 2019-02-04 17:30 ` Quentin Monnet
2019-02-04 17:35 ` Alexei Starovoitov
3 siblings, 0 replies; 8+ messages in thread
From: Quentin Monnet @ 2019-02-04 17:30 UTC (permalink / raw)
To: Yafang Shao, kafai, brakmo, ast, daniel; +Cc: netdev, shaoyafang
2019-02-03 16:15 UTC+0800 ~ Yafang Shao <laoar.shao@gmail.com>
> Then we can enable/disable socket debugging without modifying user code.
> That is more convenient for debugging.
>
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
Hi Yafang,
The list of socketopts supported by bpf_setsockopt() is described in the
documentation for this BPF helper in include/uapi/linux/bpf.h. Could you
please update that documentation, too?
Thanks,
Quentin
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH bpf-next] bpf: support SO_DEBUG in bpf_setsockopt()
2019-02-03 8:15 [PATCH bpf-next] bpf: support SO_DEBUG in bpf_setsockopt() Yafang Shao
` (2 preceding siblings ...)
2019-02-04 17:30 ` Quentin Monnet
@ 2019-02-04 17:35 ` Alexei Starovoitov
2019-02-04 20:23 ` Daniel Borkmann
2019-02-05 15:44 ` Yafang Shao
3 siblings, 2 replies; 8+ messages in thread
From: Alexei Starovoitov @ 2019-02-04 17:35 UTC (permalink / raw)
To: Yafang Shao; +Cc: kafai, brakmo, ast, daniel, netdev, shaoyafang
On Sun, Feb 03, 2019 at 04:15:07PM +0800, Yafang Shao wrote:
> Then we can enable/disable socket debugging without modifying user code.
> That is more convenient for debugging.
>
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> ---
> include/net/sock.h | 8 ++++++++
> net/core/filter.c | 3 +++
> net/core/sock.c | 8 --------
> 3 files changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/include/net/sock.h b/include/net/sock.h
> index 2b229f7..8decee9 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -1935,6 +1935,14 @@ static inline void sock_confirm_neigh(struct sk_buff *skb, struct neighbour *n)
> }
> }
>
> +static inline void sock_valbool_flag(struct sock *sk, int bit, int valbool)
> +{
> + if (valbool)
> + sock_set_flag(sk, bit);
> + else
> + sock_reset_flag(sk, bit);
> +}
> +
> bool sk_mc_loop(struct sock *sk);
>
> static inline bool sk_can_gso(const struct sock *sk)
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 3a49f68..ce5da57 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -4111,6 +4111,9 @@ static unsigned long bpf_xdp_copy(void *dst_buff, const void *src_buff,
>
> /* Only some socketops are supported */
> switch (optname) {
> + case SO_DEBUG:
> + sock_valbool_flag(sk, SOCK_DBG, val);
> + break;
I'm missing the point here.
This flag has any effect only when SOCK_DEBUGGING is set.
But it is off in distros.
Since it's for custom debug kernel only why bother with
setting the flag via bpf prog?
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH bpf-next] bpf: support SO_DEBUG in bpf_setsockopt()
2019-02-04 17:35 ` Alexei Starovoitov
@ 2019-02-04 20:23 ` Daniel Borkmann
2019-02-05 15:47 ` Yafang Shao
2019-02-05 15:44 ` Yafang Shao
1 sibling, 1 reply; 8+ messages in thread
From: Daniel Borkmann @ 2019-02-04 20:23 UTC (permalink / raw)
To: Alexei Starovoitov, Yafang Shao; +Cc: kafai, brakmo, ast, netdev, shaoyafang
On 02/04/2019 06:35 PM, Alexei Starovoitov wrote:
> On Sun, Feb 03, 2019 at 04:15:07PM +0800, Yafang Shao wrote:
>> Then we can enable/disable socket debugging without modifying user code.
>> That is more convenient for debugging.
>>
>> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
>> ---
>> include/net/sock.h | 8 ++++++++
>> net/core/filter.c | 3 +++
>> net/core/sock.c | 8 --------
>> 3 files changed, 11 insertions(+), 8 deletions(-)
>>
>> diff --git a/include/net/sock.h b/include/net/sock.h
>> index 2b229f7..8decee9 100644
>> --- a/include/net/sock.h
>> +++ b/include/net/sock.h
>> @@ -1935,6 +1935,14 @@ static inline void sock_confirm_neigh(struct sk_buff *skb, struct neighbour *n)
>> }
>> }
>>
>> +static inline void sock_valbool_flag(struct sock *sk, int bit, int valbool)
>> +{
>> + if (valbool)
>> + sock_set_flag(sk, bit);
>> + else
>> + sock_reset_flag(sk, bit);
>> +}
>> +
>> bool sk_mc_loop(struct sock *sk);
>>
>> static inline bool sk_can_gso(const struct sock *sk)
>> diff --git a/net/core/filter.c b/net/core/filter.c
>> index 3a49f68..ce5da57 100644
>> --- a/net/core/filter.c
>> +++ b/net/core/filter.c
>> @@ -4111,6 +4111,9 @@ static unsigned long bpf_xdp_copy(void *dst_buff, const void *src_buff,
>>
>> /* Only some socketops are supported */
>> switch (optname) {
>> + case SO_DEBUG:
>> + sock_valbool_flag(sk, SOCK_DBG, val);
>> + break;
>
> I'm missing the point here.
> This flag has any effect only when SOCK_DEBUGGING is set.
> But it is off in distros.
> Since it's for custom debug kernel only why bother with
> setting the flag via bpf prog?
+1, this seems like some ancient debugging interface. Back at last netconf
there was a proposal [0] to have a tcp_stats(sk, TCP_MIB_...) API for MIBs
counter such that this can be traced via BPF on a per socket basis, for
example. Might be worthwhile to work into that direction instead and potentially
get rid of the SOCK_DEBUG() statements and convert (where appropriate) to
such an interface. Thoughts?
[0] page 14, http://vger.kernel.org/netconf2018_files/BrendanGregg_netconf2018.pdf
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH bpf-next] bpf: support SO_DEBUG in bpf_setsockopt()
2019-02-04 20:23 ` Daniel Borkmann
@ 2019-02-05 15:47 ` Yafang Shao
0 siblings, 0 replies; 8+ messages in thread
From: Yafang Shao @ 2019-02-05 15:47 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Alexei Starovoitov, kafai, brakmo, ast, netdev, shaoyafang
On Tue, Feb 5, 2019 at 4:23 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 02/04/2019 06:35 PM, Alexei Starovoitov wrote:
> > On Sun, Feb 03, 2019 at 04:15:07PM +0800, Yafang Shao wrote:
> >> Then we can enable/disable socket debugging without modifying user code.
> >> That is more convenient for debugging.
> >>
> >> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> >> ---
> >> include/net/sock.h | 8 ++++++++
> >> net/core/filter.c | 3 +++
> >> net/core/sock.c | 8 --------
> >> 3 files changed, 11 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/include/net/sock.h b/include/net/sock.h
> >> index 2b229f7..8decee9 100644
> >> --- a/include/net/sock.h
> >> +++ b/include/net/sock.h
> >> @@ -1935,6 +1935,14 @@ static inline void sock_confirm_neigh(struct sk_buff *skb, struct neighbour *n)
> >> }
> >> }
> >>
> >> +static inline void sock_valbool_flag(struct sock *sk, int bit, int valbool)
> >> +{
> >> + if (valbool)
> >> + sock_set_flag(sk, bit);
> >> + else
> >> + sock_reset_flag(sk, bit);
> >> +}
> >> +
> >> bool sk_mc_loop(struct sock *sk);
> >>
> >> static inline bool sk_can_gso(const struct sock *sk)
> >> diff --git a/net/core/filter.c b/net/core/filter.c
> >> index 3a49f68..ce5da57 100644
> >> --- a/net/core/filter.c
> >> +++ b/net/core/filter.c
> >> @@ -4111,6 +4111,9 @@ static unsigned long bpf_xdp_copy(void *dst_buff, const void *src_buff,
> >>
> >> /* Only some socketops are supported */
> >> switch (optname) {
> >> + case SO_DEBUG:
> >> + sock_valbool_flag(sk, SOCK_DBG, val);
> >> + break;
> >
> > I'm missing the point here.
> > This flag has any effect only when SOCK_DEBUGGING is set.
> > But it is off in distros.
> > Since it's for custom debug kernel only why bother with
> > setting the flag via bpf prog?
>
> +1, this seems like some ancient debugging interface. Back at last netconf
> there was a proposal [0] to have a tcp_stats(sk, TCP_MIB_...) API for MIBs
> counter such that this can be traced via BPF on a per socket basis, for
> example. Might be worthwhile to work into that direction instead and potentially
> get rid of the SOCK_DEBUG() statements and convert (where appropriate) to
> such an interface. Thoughts?
>
> [0] page 14, http://vger.kernel.org/netconf2018_files/BrendanGregg_netconf2018.pdf
This proposal seems like a better solution.
I will think about it.
Thanks for your suggestion.
Thanks
Yafang
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH bpf-next] bpf: support SO_DEBUG in bpf_setsockopt()
2019-02-04 17:35 ` Alexei Starovoitov
2019-02-04 20:23 ` Daniel Borkmann
@ 2019-02-05 15:44 ` Yafang Shao
1 sibling, 0 replies; 8+ messages in thread
From: Yafang Shao @ 2019-02-05 15:44 UTC (permalink / raw)
To: Alexei Starovoitov; +Cc: kafai, brakmo, ast, daniel, netdev, shaoyafang
On Tue, Feb 5, 2019 at 1:35 AM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Sun, Feb 03, 2019 at 04:15:07PM +0800, Yafang Shao wrote:
> > Then we can enable/disable socket debugging without modifying user code.
> > That is more convenient for debugging.
> >
> > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> > ---
> > include/net/sock.h | 8 ++++++++
> > net/core/filter.c | 3 +++
> > net/core/sock.c | 8 --------
> > 3 files changed, 11 insertions(+), 8 deletions(-)
> >
> > diff --git a/include/net/sock.h b/include/net/sock.h
> > index 2b229f7..8decee9 100644
> > --- a/include/net/sock.h
> > +++ b/include/net/sock.h
> > @@ -1935,6 +1935,14 @@ static inline void sock_confirm_neigh(struct sk_buff *skb, struct neighbour *n)
> > }
> > }
> >
> > +static inline void sock_valbool_flag(struct sock *sk, int bit, int valbool)
> > +{
> > + if (valbool)
> > + sock_set_flag(sk, bit);
> > + else
> > + sock_reset_flag(sk, bit);
> > +}
> > +
> > bool sk_mc_loop(struct sock *sk);
> >
> > static inline bool sk_can_gso(const struct sock *sk)
> > diff --git a/net/core/filter.c b/net/core/filter.c
> > index 3a49f68..ce5da57 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
> > @@ -4111,6 +4111,9 @@ static unsigned long bpf_xdp_copy(void *dst_buff, const void *src_buff,
> >
> > /* Only some socketops are supported */
> > switch (optname) {
> > + case SO_DEBUG:
> > + sock_valbool_flag(sk, SOCK_DBG, val);
> > + break;
>
> I'm missing the point here.
> This flag has any effect only when SOCK_DEBUGGING is set.
> But it is off in distros.
It's not off in distros. At least it is set in RHEL.
Pls. see the comment in include/net/sock.h,
/* Define this to get the SOCK_DBG debugging facility. */
#define SOCK_DEBUGGING
> Since it's for custom debug kernel only why bother with
> setting the flag via bpf prog?
>
^ permalink raw reply [flat|nested] 8+ messages in thread