| Message ID | 20230612191641.441774-2-kuifeng@meta.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | Fix missing synack in BPF cgroup_skb filters | expand |
On Mon, Jun 12, 2023 at 12:16 PM Kui-Feng Lee <thinker.li@gmail.com> wrote: > > Always call BPF filters if CGROUP BPF is enabled for EGRESS without > checking skb->sk against sk. > > The filters were called only if sk_buff is owned by the sock that the > sk_buff is sent out through. In another words, sk_buff::sk should point to What is "sk_buff::sk" ? Did you mean skb->sk ? > the sock that it is sending through its egress. However, the filters would > miss SYNACK sk_buffs that they are owned by a request_sock but sent through > the listening sock, that is the socket listening incoming connections. > This is an unnecessary restrict. > > Signed-off-by: Kui-Feng Lee <kuifeng@meta.com> > --- > include/linux/bpf-cgroup.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/linux/bpf-cgroup.h b/include/linux/bpf-cgroup.h > index 57e9e109257e..e656da531f9f 100644 > --- a/include/linux/bpf-cgroup.h > +++ b/include/linux/bpf-cgroup.h > @@ -199,7 +199,7 @@ static inline bool cgroup_bpf_sock_enabled(struct sock *sk, > #define BPF_CGROUP_RUN_PROG_INET_EGRESS(sk, skb) \ > ({ \ > int __ret = 0; \ > - if (cgroup_bpf_enabled(CGROUP_INET_EGRESS) && sk && sk == skb->sk) { \ > + if (cgroup_bpf_enabled(CGROUP_INET_EGRESS) && sk) { I did a bit of git-archeology. That check was there since the beginning of cgroup-bpf and came as a suggestion to use 'sk' instead of 'skb->sk': https://lore.kernel.org/all/58193E9D.7040201@iogearbox.net/ Using sk is certainly correct. It looks to me that the check was added just for a "piece of mind".
On 6/12/23 13:17, Alexei Starovoitov wrote: > On Mon, Jun 12, 2023 at 12:16 PM Kui-Feng Lee <thinker.li@gmail.com> wrote: >> >> Always call BPF filters if CGROUP BPF is enabled for EGRESS without >> checking skb->sk against sk. >> >> The filters were called only if sk_buff is owned by the sock that the >> sk_buff is sent out through. In another words, sk_buff::sk should point to > > What is "sk_buff::sk" ? Did you mean skb->sk ? Yes! > >> the sock that it is sending through its egress. However, the filters would >> miss SYNACK sk_buffs that they are owned by a request_sock but sent through >> the listening sock, that is the socket listening incoming connections. >> This is an unnecessary restrict. >> >> Signed-off-by: Kui-Feng Lee <kuifeng@meta.com> >> --- >> include/linux/bpf-cgroup.h | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/include/linux/bpf-cgroup.h b/include/linux/bpf-cgroup.h >> index 57e9e109257e..e656da531f9f 100644 >> --- a/include/linux/bpf-cgroup.h >> +++ b/include/linux/bpf-cgroup.h >> @@ -199,7 +199,7 @@ static inline bool cgroup_bpf_sock_enabled(struct sock *sk, >> #define BPF_CGROUP_RUN_PROG_INET_EGRESS(sk, skb) \ >> ({ \ >> int __ret = 0; \ >> - if (cgroup_bpf_enabled(CGROUP_INET_EGRESS) && sk && sk == skb->sk) { \ >> + if (cgroup_bpf_enabled(CGROUP_INET_EGRESS) && sk) { > > > I did a bit of git-archeology. > That check was there since the beginning of cgroup-bpf and > came as a suggestion to use 'sk' instead of 'skb->sk': > https://lore.kernel.org/all/58193E9D.7040201@iogearbox.net/ > > Using sk is certainly correct. It looks to me that the check > was added just for a "piece of mind". > Good to know that. Thank you for the confirmation.
diff --git a/include/linux/bpf-cgroup.h b/include/linux/bpf-cgroup.h index 57e9e109257e..e656da531f9f 100644 --- a/include/linux/bpf-cgroup.h +++ b/include/linux/bpf-cgroup.h @@ -199,7 +199,7 @@ static inline bool cgroup_bpf_sock_enabled(struct sock *sk, #define BPF_CGROUP_RUN_PROG_INET_EGRESS(sk, skb) \ ({ \ int __ret = 0; \ - if (cgroup_bpf_enabled(CGROUP_INET_EGRESS) && sk && sk == skb->sk) { \ + if (cgroup_bpf_enabled(CGROUP_INET_EGRESS) && sk) { \ typeof(sk) __sk = sk_to_full_sk(sk); \ if (sk_fullsock(__sk) && \ cgroup_bpf_sock_enabled(__sk, CGROUP_INET_EGRESS)) \
Always call BPF filters if CGROUP BPF is enabled for EGRESS without checking skb->sk against sk. The filters were called only if sk_buff is owned by the sock that the sk_buff is sent out through. In another words, sk_buff::sk should point to the sock that it is sending through its egress. However, the filters would miss SYNACK sk_buffs that they are owned by a request_sock but sent through the listening sock, that is the socket listening incoming connections. This is an unnecessary restrict. Signed-off-by: Kui-Feng Lee <kuifeng@meta.com> --- include/linux/bpf-cgroup.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)