| Message ID | 1549181707-16864-1-git-send-email-laoar.shao@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [bpf-next] bpf: support SO_DEBUG in bpf_setsockopt() | expand |
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 >
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
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
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?
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
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? >
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
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())
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(-)