* [PATCH bpf-next v2] Only run BPF cgroup unix sockaddr recvmsg() hooks on named sockets
@ 2023-10-12 8:52 Daan De Meyer
2023-10-12 18:11 ` Kuniyuki Iwashima
0 siblings, 1 reply; 4+ messages in thread
From: Daan De Meyer @ 2023-10-12 8:52 UTC (permalink / raw)
To: bpf; +Cc: Daan De Meyer, martin.lau, kernel-team, netdev
Changes since v1:
* Added missing Signed-off-by tag
We should not run the recvmsg() hooks on unnamed sockets as we do
not run them on unnamed sockets in the other hooks either. We may
look into relaxing this later but for now let's make sure we are
consistent and not run the hooks on unnamed sockets anywhere.
Signed-off-by: Daan De Meyer <daan.j.demeyer@gmail.com>
---
net/unix/af_unix.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index e10d07c76044..81fb8bddaff9 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -2416,9 +2416,10 @@ int __unix_dgram_recvmsg(struct sock *sk, struct msghdr *msg, size_t size,
if (msg->msg_name) {
unix_copy_addr(msg, skb->sk);
- BPF_CGROUP_RUN_PROG_UNIX_RECVMSG_LOCK(sk,
- msg->msg_name,
- &msg->msg_namelen);
+ if (msg->msg_namelen > 0)
+ BPF_CGROUP_RUN_PROG_UNIX_RECVMSG_LOCK(sk,
+ msg->msg_name,
+ &msg->msg_namelen);
}
if (size > skb->len - skip)
@@ -2773,9 +2774,10 @@ static int unix_stream_read_generic(struct unix_stream_read_state *state,
state->msg->msg_name);
unix_copy_addr(state->msg, skb->sk);
- BPF_CGROUP_RUN_PROG_UNIX_RECVMSG_LOCK(sk,
- state->msg->msg_name,
- &state->msg->msg_namelen);
+ if (state->msg->msg_namelen > 0)
+ BPF_CGROUP_RUN_PROG_UNIX_RECVMSG_LOCK(sk,
+ state->msg->msg_name,
+ &state->msg->msg_namelen);
sunaddr = NULL;
}
--
2.41.0
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH bpf-next v2] Only run BPF cgroup unix sockaddr recvmsg() hooks on named sockets
2023-10-12 8:52 [PATCH bpf-next v2] Only run BPF cgroup unix sockaddr recvmsg() hooks on named sockets Daan De Meyer
@ 2023-10-12 18:11 ` Kuniyuki Iwashima
2023-10-16 18:33 ` Martin KaFai Lau
0 siblings, 1 reply; 4+ messages in thread
From: Kuniyuki Iwashima @ 2023-10-12 18:11 UTC (permalink / raw)
To: daan.j.demeyer; +Cc: bpf, kernel-team, martin.lau, netdev, kuniyu
From: Daan De Meyer <daan.j.demeyer@gmail.com>
Date: Thu, 12 Oct 2023 10:52:13 +0200
> Changes since v1:
>
> * Added missing Signed-off-by tag
You can put these after --- so that it will disappear when merged.
>
> We should not run the recvmsg() hooks on unnamed sockets as we do
> not run them on unnamed sockets in the other hooks either. We may
> look into relaxing this later but for now let's make sure we are
> consistent and not run the hooks on unnamed sockets anywhere.
>
> Signed-off-by: Daan De Meyer <daan.j.demeyer@gmail.com>
> ---
> net/unix/af_unix.c | 14 ++++++++------
> 1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> index e10d07c76044..81fb8bddaff9 100644
> --- a/net/unix/af_unix.c
> +++ b/net/unix/af_unix.c
> @@ -2416,9 +2416,10 @@ int __unix_dgram_recvmsg(struct sock *sk, struct msghdr *msg, size_t size,
> if (msg->msg_name) {
> unix_copy_addr(msg, skb->sk);
How is an unnamed socket set to skb->sk ?
>
> - BPF_CGROUP_RUN_PROG_UNIX_RECVMSG_LOCK(sk,
> - msg->msg_name,
> - &msg->msg_namelen);
> + if (msg->msg_namelen > 0)
> + BPF_CGROUP_RUN_PROG_UNIX_RECVMSG_LOCK(sk,
> + msg->msg_name,
> + &msg->msg_namelen);
> }
>
> if (size > skb->len - skip)
> @@ -2773,9 +2774,10 @@ static int unix_stream_read_generic(struct unix_stream_read_state *state,
> state->msg->msg_name);
> unix_copy_addr(state->msg, skb->sk);
>
> - BPF_CGROUP_RUN_PROG_UNIX_RECVMSG_LOCK(sk,
> - state->msg->msg_name,
> - &state->msg->msg_namelen);
> + if (state->msg->msg_namelen > 0)
> + BPF_CGROUP_RUN_PROG_UNIX_RECVMSG_LOCK(sk,
> + state->msg->msg_name,
> + &state->msg->msg_namelen);
>
> sunaddr = NULL;
> }
> --
> 2.41.0
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH bpf-next v2] Only run BPF cgroup unix sockaddr recvmsg() hooks on named sockets
2023-10-12 18:11 ` Kuniyuki Iwashima
@ 2023-10-16 18:33 ` Martin KaFai Lau
2023-10-16 18:47 ` Kuniyuki Iwashima
0 siblings, 1 reply; 4+ messages in thread
From: Martin KaFai Lau @ 2023-10-16 18:33 UTC (permalink / raw)
To: Kuniyuki Iwashima, daan.j.demeyer; +Cc: bpf, kernel-team, netdev
On 10/12/23 11:11 AM, Kuniyuki Iwashima wrote:
> From: Daan De Meyer <daan.j.demeyer@gmail.com>
> Date: Thu, 12 Oct 2023 10:52:13 +0200
>> Changes since v1:
>>
>> * Added missing Signed-off-by tag
>
> You can put these after --- so that it will disappear when merged.
>
>
>>
>> We should not run the recvmsg() hooks on unnamed sockets as we do
>> not run them on unnamed sockets in the other hooks either. We may
>> look into relaxing this later but for now let's make sure we are
>> consistent and not run the hooks on unnamed sockets anywhere.
>>
>> Signed-off-by: Daan De Meyer <daan.j.demeyer@gmail.com>
>> ---
>> net/unix/af_unix.c | 14 ++++++++------
>> 1 file changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
>> index e10d07c76044..81fb8bddaff9 100644
>> --- a/net/unix/af_unix.c
>> +++ b/net/unix/af_unix.c
>> @@ -2416,9 +2416,10 @@ int __unix_dgram_recvmsg(struct sock *sk, struct msghdr *msg, size_t size,
>> if (msg->msg_name) {
>> unix_copy_addr(msg, skb->sk);
>
> How is an unnamed socket set to skb->sk ?
I had a similar question. Most likely socketpair? Please add an explanation in
the commit message in v3. Please also help to add a selftest for this case.
>
>
>>
>> - BPF_CGROUP_RUN_PROG_UNIX_RECVMSG_LOCK(sk,
>> - msg->msg_name,
>> - &msg->msg_namelen);
>> + if (msg->msg_namelen > 0)
>> + BPF_CGROUP_RUN_PROG_UNIX_RECVMSG_LOCK(sk,
>> + msg->msg_name,
>> + &msg->msg_namelen);
>> }
>>
>> if (size > skb->len - skip)
>> @@ -2773,9 +2774,10 @@ static int unix_stream_read_generic(struct unix_stream_read_state *state,
>> state->msg->msg_name);
>> unix_copy_addr(state->msg, skb->sk);
>>
>> - BPF_CGROUP_RUN_PROG_UNIX_RECVMSG_LOCK(sk,
>> - state->msg->msg_name,
>> - &state->msg->msg_namelen);
>> + if (state->msg->msg_namelen > 0)
>> + BPF_CGROUP_RUN_PROG_UNIX_RECVMSG_LOCK(sk,
>> + state->msg->msg_name,
>> + &state->msg->msg_namelen);
>>
>> sunaddr = NULL;
>> }
>> --
>> 2.41.0
>>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH bpf-next v2] Only run BPF cgroup unix sockaddr recvmsg() hooks on named sockets
2023-10-16 18:33 ` Martin KaFai Lau
@ 2023-10-16 18:47 ` Kuniyuki Iwashima
0 siblings, 0 replies; 4+ messages in thread
From: Kuniyuki Iwashima @ 2023-10-16 18:47 UTC (permalink / raw)
To: martin.lau; +Cc: bpf, daan.j.demeyer, kernel-team, kuniyu, netdev
From: Martin KaFai Lau <martin.lau@linux.dev>
Date: Mon, 16 Oct 2023 11:33:36 -0700
> On 10/12/23 11:11 AM, Kuniyuki Iwashima wrote:
> > From: Daan De Meyer <daan.j.demeyer@gmail.com>
> > Date: Thu, 12 Oct 2023 10:52:13 +0200
> >> Changes since v1:
> >>
> >> * Added missing Signed-off-by tag
> >
> > You can put these after --- so that it will disappear when merged.
> >
> >
> >>
> >> We should not run the recvmsg() hooks on unnamed sockets as we do
> >> not run them on unnamed sockets in the other hooks either. We may
> >> look into relaxing this later but for now let's make sure we are
> >> consistent and not run the hooks on unnamed sockets anywhere.
> >>
> >> Signed-off-by: Daan De Meyer <daan.j.demeyer@gmail.com>
> >> ---
> >> net/unix/af_unix.c | 14 ++++++++------
> >> 1 file changed, 8 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> >> index e10d07c76044..81fb8bddaff9 100644
> >> --- a/net/unix/af_unix.c
> >> +++ b/net/unix/af_unix.c
> >> @@ -2416,9 +2416,10 @@ int __unix_dgram_recvmsg(struct sock *sk, struct msghdr *msg, size_t size,
> >> if (msg->msg_name) {
> >> unix_copy_addr(msg, skb->sk);
> >
> > How is an unnamed socket set to skb->sk ?
>
> I had a similar question. Most likely socketpair? Please add an explanation in
> the commit message in v3. Please also help to add a selftest for this case.
Ah exactly, socketpair() for SOCK_STREAM does it.
>
> >
> >
> >>
> >> - BPF_CGROUP_RUN_PROG_UNIX_RECVMSG_LOCK(sk,
> >> - msg->msg_name,
> >> - &msg->msg_namelen);
> >> + if (msg->msg_namelen > 0)
> >> + BPF_CGROUP_RUN_PROG_UNIX_RECVMSG_LOCK(sk,
> >> + msg->msg_name,
> >> + &msg->msg_namelen);
> >> }
> >>
> >> if (size > skb->len - skip)
> >> @@ -2773,9 +2774,10 @@ static int unix_stream_read_generic(struct unix_stream_read_state *state,
> >> state->msg->msg_name);
> >> unix_copy_addr(state->msg, skb->sk);
> >>
> >> - BPF_CGROUP_RUN_PROG_UNIX_RECVMSG_LOCK(sk,
> >> - state->msg->msg_name,
> >> - &state->msg->msg_namelen);
> >> + if (state->msg->msg_namelen > 0)
> >> + BPF_CGROUP_RUN_PROG_UNIX_RECVMSG_LOCK(sk,
> >> + state->msg->msg_name,
> >> + &state->msg->msg_namelen);
> >>
> >> sunaddr = NULL;
> >> }
> >> --
> >> 2.41.0
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-10-16 18:47 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-10-12 8:52 [PATCH bpf-next v2] Only run BPF cgroup unix sockaddr recvmsg() hooks on named sockets Daan De Meyer
2023-10-12 18:11 ` Kuniyuki Iwashima
2023-10-16 18:33 ` Martin KaFai Lau
2023-10-16 18:47 ` Kuniyuki Iwashima
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).