| Message ID | 20240923170322.535940-1-sahandevs@gmail.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: expose __sock_sendmsg() symbol | expand |
Should this be backported? I'm wondering if this needs a "Fixes" tag. -Jordan On Mon, Sep 23, 2024 at 10:03 AM Sahand <sahandevs@gmail.com> wrote: > > From: Sahand Akbarzadeh <sahandevs@gmail.com> > > Commit 86a7e0b69bd5b812e48a20c66c2161744f3caa16 ("net: prevent rewrite > of msg_name in sock_sendmsg()") moved the original implementation of > sock_sendmsg() to __sock_sendmsg() and made sock_sendmsg() a wrapper > with extra checks. However, __sys_sendto() still uses __sock_sendmsg() > directly, causing BPF programs attached to kprobe:sock_sendmsg() to not > trigger on sendto() calls. > > This patch exposes the __sock_sendmsg() symbol to allow writing BPF > programs similar to those for older kernels. > > Signed-off-by: Sahand Akbarzadeh <sahandevs@gmail.com> > --- > include/linux/net.h | 1 + > net/socket.c | 2 +- > 2 files changed, 2 insertions(+), 1 deletion(-) > > diff --git a/include/linux/net.h b/include/linux/net.h > index b75bc534c..983be8a14 100644 > --- a/include/linux/net.h > +++ b/include/linux/net.h > @@ -258,6 +258,7 @@ int sock_create_kern(struct net *net, int family, int type, int proto, struct so > int sock_create_lite(int family, int type, int proto, struct socket **res); > struct socket *sock_alloc(void); > void sock_release(struct socket *sock); > +int __sock_sendmsg(struct socket *sock, struct msghdr *msg); > int sock_sendmsg(struct socket *sock, struct msghdr *msg); > int sock_recvmsg(struct socket *sock, struct msghdr *msg, int flags); > struct file *sock_alloc_file(struct socket *sock, int flags, const char *dname); > diff --git a/net/socket.c b/net/socket.c > index 8d8b84fa4..5c790205d 100644 > --- a/net/socket.c > +++ b/net/socket.c > @@ -737,7 +737,7 @@ static inline int sock_sendmsg_nosec(struct socket *sock, struct msghdr *msg) > return ret; > } > > -static int __sock_sendmsg(struct socket *sock, struct msghdr *msg) > +int __sock_sendmsg(struct socket *sock, struct msghdr *msg) > { > int err = security_socket_sendmsg(sock, msg, > msg_data_left(msg)); > -- > 2.43.0 >
On Mon, Sep 23, 2024 at 7:48 PM Jordan Rife <jrife@google.com> wrote: > > Should this be backported? I'm wondering if this needs a "Fixes" tag. > > -Jordan > > On Mon, Sep 23, 2024 at 10:03 AM Sahand <sahandevs@gmail.com> wrote: > > > > From: Sahand Akbarzadeh <sahandevs@gmail.com> > > > > Commit 86a7e0b69bd5b812e48a20c66c2161744f3caa16 ("net: prevent rewrite > > of msg_name in sock_sendmsg()") moved the original implementation of > > sock_sendmsg() to __sock_sendmsg() and made sock_sendmsg() a wrapper > > with extra checks. However, __sys_sendto() still uses __sock_sendmsg() > > directly, causing BPF programs attached to kprobe:sock_sendmsg() to not > > trigger on sendto() calls. > > > > This patch exposes the __sock_sendmsg() symbol to allow writing BPF > > programs similar to those for older kernels. > > > > Signed-off-by: Sahand Akbarzadeh <sahandevs@gmail.com> > > --- > > include/linux/net.h | 1 + > > net/socket.c | 2 +- > > 2 files changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/include/linux/net.h b/include/linux/net.h > > index b75bc534c..983be8a14 100644 > > --- a/include/linux/net.h > > +++ b/include/linux/net.h > > @@ -258,6 +258,7 @@ int sock_create_kern(struct net *net, int family, int type, int proto, struct so > > int sock_create_lite(int family, int type, int proto, struct socket **res); > > struct socket *sock_alloc(void); > > void sock_release(struct socket *sock); > > +int __sock_sendmsg(struct socket *sock, struct msghdr *msg); > > int sock_sendmsg(struct socket *sock, struct msghdr *msg); > > int sock_recvmsg(struct socket *sock, struct msghdr *msg, int flags); > > struct file *sock_alloc_file(struct socket *sock, int flags, const char *dname); > > diff --git a/net/socket.c b/net/socket.c > > index 8d8b84fa4..5c790205d 100644 > > --- a/net/socket.c > > +++ b/net/socket.c > > @@ -737,7 +737,7 @@ static inline int sock_sendmsg_nosec(struct socket *sock, struct msghdr *msg) > > return ret; > > } > > > > -static int __sock_sendmsg(struct socket *sock, struct msghdr *msg) > > +int __sock_sendmsg(struct socket *sock, struct msghdr *msg) > > { > > int err = security_socket_sendmsg(sock, msg, > > msg_data_left(msg)); > > -- > > 2.43.0 > > Old programs were using kprobe:sock_sendmsg How will this patch restore the operations ? It seems programs have to use kprobe:__sock_sendmsg even after this patch ? Please provide a more precise changelog. I _think_ that perf does not care : # perf probe -a __sock_sendmsg Added new events: probe:__sock_sendmsg (on __sock_sendmsg) probe:__sock_sendmsg (on __sock_sendmsg) probe:__sock_sendmsg (on __sock_sendmsg) probe:__sock_sendmsg (on __sock_sendmsg) You can now use it in all perf tools, such as: perf record -e probe:__sock_sendmsg -aR sleep 1
On Mon, Sep 23, 2024 at 8:45 PM Sahand Akbarzadeh <sahandevs@gmail.com> wrote: > > Yes, existing program still need some modification in order to work and > are already broken (from kernel 6.8 to master branch) for some time. The issue > here is there is no direct probe equivalent one could use to update those scripts. > > By adding `__sock_sendmsg`, one could attach based on kernel version or do something > like this: > > sudo bpftrace -e 'kprobe:sock_sendmsg,kprobe:__sock_sendmsg {}' > > which only throws a warning if it can't find the `__sock_sendmsg` > > - Sahand Convention on netdev mailing list is to not top post. Removing the static is not enough, a compiler and linker can completely inline / delete this function. Anyway, I do not think sock_sendmsg() was part of any ABI. If it was ABI, we would have to reinstate sock_sendmsg(), not making __sock_sendmsg() visible.
On Mon, Sep 23, 2024 at 10:30 PM Eric Dumazet <edumazet@google.com> wrote: > > On Mon, Sep 23, 2024 at 8:45 PM Sahand Akbarzadeh <sahandevs@gmail.com> wrote: > > > > Yes, existing program still need some modification in order to work and > > are already broken (from kernel 6.8 to master branch) for some time. The issue > > here is there is no direct probe equivalent one could use to update those scripts. > > > > By adding `__sock_sendmsg`, one could attach based on kernel version or do something > > like this: > > > > sudo bpftrace -e 'kprobe:sock_sendmsg,kprobe:__sock_sendmsg {}' > > > > which only throws a warning if it can't find the `__sock_sendmsg` > > > > - Sahand > > Convention on netdev mailing list is to not top post. > > Removing the static is not enough, a compiler and linker can > completely inline / delete this function. > > Anyway, I do not think sock_sendmsg() was part of any ABI. > > If it was ABI, we would have to reinstate sock_sendmsg(), not making > __sock_sendmsg() visible. Sorry about the top posting. I do think this patch is not necessarily a good solution to the problem but I'm not sure what is a/the good solution for it. To give more context, I was trying to figure out why this observability script (written in bpftrace) doesn't work on some kernels and how to fix it. (goal: calculating network usage per process per thread. recv part works fine) kretprobe:sock_sendmsg { if (@inetsocket[tid] && retval < 0x7fffffff) { @send_bytes[pid, comm, tid] = sum(retval); } delete(@inetsocket[tid]) } Script Source: https://www.gcardone.net/2020-07-31-per-process-bandwidth-monitoring-on-Linux-with-bpftrace/ > Removing the static is not enough Should I also add a EXPORT_SYMBOL?
On 9/23/24 21:36, Sahand Evs wrote: > On Mon, Sep 23, 2024 at 10:30 PM Eric Dumazet <edumazet@google.com> wrote: >> Convention on netdev mailing list is to not top post. >> >> Removing the static is not enough, a compiler and linker can >> completely inline / delete this function. >> >> Anyway, I do not think sock_sendmsg() was part of any ABI. [...] >> Removing the static is not enough > Should I also add a EXPORT_SYMBOL? No, the relevant part is that the kernel don't guarantee __sock_sendmsg()/sock_sendmsg() being constant (or present) across different kernel versions (it's not part of any uAPI/ABI contract) The user-space has to cope with that, trying to attach to different function, according to what is available and visible. Possibly you don't see __sock_sendmsg because you don't have the kernel debug infos available. Try install them for the running kernel. Cheers, Paolo
diff --git a/include/linux/net.h b/include/linux/net.h index b75bc534c..983be8a14 100644 --- a/include/linux/net.h +++ b/include/linux/net.h @@ -258,6 +258,7 @@ int sock_create_kern(struct net *net, int family, int type, int proto, struct so int sock_create_lite(int family, int type, int proto, struct socket **res); struct socket *sock_alloc(void); void sock_release(struct socket *sock); +int __sock_sendmsg(struct socket *sock, struct msghdr *msg); int sock_sendmsg(struct socket *sock, struct msghdr *msg); int sock_recvmsg(struct socket *sock, struct msghdr *msg, int flags); struct file *sock_alloc_file(struct socket *sock, int flags, const char *dname); diff --git a/net/socket.c b/net/socket.c index 8d8b84fa4..5c790205d 100644 --- a/net/socket.c +++ b/net/socket.c @@ -737,7 +737,7 @@ static inline int sock_sendmsg_nosec(struct socket *sock, struct msghdr *msg) return ret; } -static int __sock_sendmsg(struct socket *sock, struct msghdr *msg) +int __sock_sendmsg(struct socket *sock, struct msghdr *msg) { int err = security_socket_sendmsg(sock, msg, msg_data_left(msg));