| Message ID | 20230418153148.2231644-3-aditi.ghag@isovalent.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf: Add socket destroy capability | expand |
On 4/18/23 8:31 AM, Aditi Ghag wrote: > This is a preparatory commit to remove the field. The field was > previously shared between proc fs and BPF UDP socket iterators. As the > follow-up commits will decouple the implementation for the iterators, > remove the field. As for BPF socket iterator, filtering of sockets is > exepected to be done in BPF programs. > > Suggested-by: Martin KaFai Lau <martin.lau@kernel.org> > Signed-off-by: Aditi Ghag <aditi.ghag@isovalent.com> > --- > include/net/udp.h | 1 - > net/ipv4/udp.c | 34 ++++------------------------------ > 2 files changed, 4 insertions(+), 31 deletions(-) > > diff --git a/include/net/udp.h b/include/net/udp.h > index de4b528522bb..5cad44318d71 100644 > --- a/include/net/udp.h > +++ b/include/net/udp.h > @@ -437,7 +437,6 @@ struct udp_seq_afinfo { > struct udp_iter_state { > struct seq_net_private p; > int bucket; > - struct udp_seq_afinfo *bpf_seq_afinfo; > }; > > void *udp_seq_start(struct seq_file *seq, loff_t *pos); > diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c > index c605d171eb2d..3c9eeee28678 100644 > --- a/net/ipv4/udp.c > +++ b/net/ipv4/udp.c > @@ -2997,10 +2997,7 @@ static struct sock *udp_get_first(struct seq_file *seq, int start) > struct udp_table *udptable; > struct sock *sk; > > - if (state->bpf_seq_afinfo) > - afinfo = state->bpf_seq_afinfo; > - else > - afinfo = pde_data(file_inode(seq->file)); > + afinfo = pde_data(file_inode(seq->file)); I can see how this change will work after patch 4. However, this patch alone cannot work independently as is. The udp bpf iter still uses the udp_get_{first,next} and udp_seq_stop() up-to this patch. First, patch 3 refactoring should be done before patch 2 here. The removal of 'struct udp_seq_afinfo *bpf_seq_afinfo' in patch 2 should be done when all the necessary refactoring is in-place first. Also, this afinfo is passed to udp_get_table_afinfo(). How about renaming udp_get_table_afinfo() to udp_get_table_seq() and having it take the "seq" as the arg instead. This probably will deserve another refactoring patch before finally removing bpf_seq_afinfo. Something like this (un-compiled code): static struct udp_table *udp_get_table_seq(struct seq_file *seq, struct net *net) { const struct udp_seq_afinfo *afinfo; if (st->bpf_seq_afinfo) return net->ipv4.udp_table; afinfo = pde_data(file_inode(seq->file)); return afinfo->udp_table ? : net->ipv4.udp_table; } Of course, when the later patch finally removes the bpf_seq_afinfo, the 'if (st->bpf_seq_afinfo)' test should be replaced with the 'if (seq->op == &bpf_iter_udp_seq_ops)' test. That will also make the afinfo dance in bpf_iter_udp_batch() in patch 4 goes away. > > udptable = udp_get_table_afinfo(afinfo, net); > > @@ -3033,10 +3030,7 @@ static struct sock *udp_get_next(struct seq_file *seq, struct sock *sk) > struct udp_seq_afinfo *afinfo; > struct udp_table *udptable; > > - if (state->bpf_seq_afinfo) > - afinfo = state->bpf_seq_afinfo; > - else > - afinfo = pde_data(file_inode(seq->file)); > + afinfo = pde_data(file_inode(seq->file)); > > do { > sk = sk_next(sk); > @@ -3094,10 +3088,7 @@ void udp_seq_stop(struct seq_file *seq, void *v) > struct udp_seq_afinfo *afinfo; > struct udp_table *udptable; > > - if (state->bpf_seq_afinfo) > - afinfo = state->bpf_seq_afinfo; > - else > - afinfo = pde_data(file_inode(seq->file)); > + afinfo = pde_data(file_inode(seq->file)); > > udptable = udp_get_table_afinfo(afinfo, seq_file_net(seq)); > > @@ -3415,28 +3406,11 @@ DEFINE_BPF_ITER_FUNC(udp, struct bpf_iter_meta *meta, > > static int bpf_iter_init_udp(void *priv_data, struct bpf_iter_aux_info *aux) > { > - struct udp_iter_state *st = priv_data; > - struct udp_seq_afinfo *afinfo; > - int ret; > - > - afinfo = kmalloc(sizeof(*afinfo), GFP_USER | __GFP_NOWARN); > - if (!afinfo) > - return -ENOMEM; > - > - afinfo->family = AF_UNSPEC; > - afinfo->udp_table = NULL; > - st->bpf_seq_afinfo = afinfo; > - ret = bpf_iter_init_seq_net(priv_data, aux); > - if (ret) > - kfree(afinfo); > - return ret; > + return bpf_iter_init_seq_net(priv_data, aux); Nice simplification with the bpf_seq_afinfo cleanup. > } > > static void bpf_iter_fini_udp(void *priv_data) > { > - struct udp_iter_state *st = priv_data; > - > - kfree(st->bpf_seq_afinfo); > bpf_iter_fini_seq_net(priv_data); > } >
> On Apr 23, 2023, at 5:18 PM, Martin KaFai Lau <martin.lau@linux.dev> wrote: > > On 4/18/23 8:31 AM, Aditi Ghag wrote: >> This is a preparatory commit to remove the field. The field was >> previously shared between proc fs and BPF UDP socket iterators. As the >> follow-up commits will decouple the implementation for the iterators, >> remove the field. As for BPF socket iterator, filtering of sockets is >> exepected to be done in BPF programs. >> Suggested-by: Martin KaFai Lau <martin.lau@kernel.org> >> Signed-off-by: Aditi Ghag <aditi.ghag@isovalent.com> >> --- >> include/net/udp.h | 1 - >> net/ipv4/udp.c | 34 ++++------------------------------ >> 2 files changed, 4 insertions(+), 31 deletions(-) >> diff --git a/include/net/udp.h b/include/net/udp.h >> index de4b528522bb..5cad44318d71 100644 >> --- a/include/net/udp.h >> +++ b/include/net/udp.h >> @@ -437,7 +437,6 @@ struct udp_seq_afinfo { >> struct udp_iter_state { >> struct seq_net_private p; >> int bucket; >> - struct udp_seq_afinfo *bpf_seq_afinfo; >> }; >> void *udp_seq_start(struct seq_file *seq, loff_t *pos); >> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c >> index c605d171eb2d..3c9eeee28678 100644 >> --- a/net/ipv4/udp.c >> +++ b/net/ipv4/udp.c >> @@ -2997,10 +2997,7 @@ static struct sock *udp_get_first(struct seq_file *seq, int start) >> struct udp_table *udptable; >> struct sock *sk; >> - if (state->bpf_seq_afinfo) >> - afinfo = state->bpf_seq_afinfo; >> - else >> - afinfo = pde_data(file_inode(seq->file)); >> + afinfo = pde_data(file_inode(seq->file)); > > I can see how this change will work after patch 4. However, this patch alone cannot work independently as is. The udp bpf iter still uses the udp_get_{first,next} and udp_seq_stop() up-to this patch. > > First, patch 3 refactoring should be done before patch 2 here. The removal of 'struct udp_seq_afinfo *bpf_seq_afinfo' in patch 2 should be done when all the necessary refactoring is in-place first. > > Also, this afinfo is passed to udp_get_table_afinfo(). How about renaming udp_get_table_afinfo() to udp_get_table_seq() and having it take the "seq" as the arg instead. This probably will deserve another refactoring patch before finally removing bpf_seq_afinfo. Something like this (un-compiled code): > > static struct udp_table *udp_get_table_seq(struct seq_file *seq, > struct net *net) > { > const struct udp_seq_afinfo *afinfo; > > if (st->bpf_seq_afinfo) > return net->ipv4.udp_table; > > afinfo = pde_data(file_inode(seq->file)); > return afinfo->udp_table ? : net->ipv4.udp_table; > } > > Of course, when the later patch finally removes the bpf_seq_afinfo, the 'if (st->bpf_seq_afinfo)' test should be replaced with the 'if (seq->op == &bpf_iter_udp_seq_ops)' test. > > That will also make the afinfo dance in bpf_iter_udp_batch() in patch 4 goes away. Sweet! I suppose it was worth resolving a few conflicts while creating the new preparatory patch, especially since the refactoring simplified unnecessary setting of afinfo in bpf_iter_udp_batch(). The additional minor change that was needed was to forward declare bpf_iter_udp_seq_ops. And of course, the if (seq->op == &bpf_iter_udp_seq_ops) check needed to be wrapped in the CONFIG_BPF_SYSCALL ifdef. > >> udptable = udp_get_table_afinfo(afinfo, net); >> @@ -3033,10 +3030,7 @@ static struct sock *udp_get_next(struct seq_file *seq, struct sock *sk) >> struct udp_seq_afinfo *afinfo; >> struct udp_table *udptable; >> - if (state->bpf_seq_afinfo) >> - afinfo = state->bpf_seq_afinfo; >> - else >> - afinfo = pde_data(file_inode(seq->file)); >> + afinfo = pde_data(file_inode(seq->file)); >> do { >> sk = sk_next(sk); >> @@ -3094,10 +3088,7 @@ void udp_seq_stop(struct seq_file *seq, void *v) >> struct udp_seq_afinfo *afinfo; >> struct udp_table *udptable; >> - if (state->bpf_seq_afinfo) >> - afinfo = state->bpf_seq_afinfo; >> - else >> - afinfo = pde_data(file_inode(seq->file)); >> + afinfo = pde_data(file_inode(seq->file)); >> udptable = udp_get_table_afinfo(afinfo, seq_file_net(seq)); >> @@ -3415,28 +3406,11 @@ DEFINE_BPF_ITER_FUNC(udp, struct bpf_iter_meta *meta, >> static int bpf_iter_init_udp(void *priv_data, struct bpf_iter_aux_info *aux) >> { >> - struct udp_iter_state *st = priv_data; >> - struct udp_seq_afinfo *afinfo; >> - int ret; >> - >> - afinfo = kmalloc(sizeof(*afinfo), GFP_USER | __GFP_NOWARN); >> - if (!afinfo) >> - return -ENOMEM; >> - >> - afinfo->family = AF_UNSPEC; >> - afinfo->udp_table = NULL; >> - st->bpf_seq_afinfo = afinfo; >> - ret = bpf_iter_init_seq_net(priv_data, aux); >> - if (ret) >> - kfree(afinfo); >> - return ret; >> + return bpf_iter_init_seq_net(priv_data, aux); > > Nice simplification with the bpf_seq_afinfo cleanup. > >> } >> static void bpf_iter_fini_udp(void *priv_data) >> { >> - struct udp_iter_state *st = priv_data; >> - >> - kfree(st->bpf_seq_afinfo); >> bpf_iter_fini_seq_net(priv_data); >> }
diff --git a/include/net/udp.h b/include/net/udp.h index de4b528522bb..5cad44318d71 100644 --- a/include/net/udp.h +++ b/include/net/udp.h @@ -437,7 +437,6 @@ struct udp_seq_afinfo { struct udp_iter_state { struct seq_net_private p; int bucket; - struct udp_seq_afinfo *bpf_seq_afinfo; }; void *udp_seq_start(struct seq_file *seq, loff_t *pos); diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c index c605d171eb2d..3c9eeee28678 100644 --- a/net/ipv4/udp.c +++ b/net/ipv4/udp.c @@ -2997,10 +2997,7 @@ static struct sock *udp_get_first(struct seq_file *seq, int start) struct udp_table *udptable; struct sock *sk; - if (state->bpf_seq_afinfo) - afinfo = state->bpf_seq_afinfo; - else - afinfo = pde_data(file_inode(seq->file)); + afinfo = pde_data(file_inode(seq->file)); udptable = udp_get_table_afinfo(afinfo, net); @@ -3033,10 +3030,7 @@ static struct sock *udp_get_next(struct seq_file *seq, struct sock *sk) struct udp_seq_afinfo *afinfo; struct udp_table *udptable; - if (state->bpf_seq_afinfo) - afinfo = state->bpf_seq_afinfo; - else - afinfo = pde_data(file_inode(seq->file)); + afinfo = pde_data(file_inode(seq->file)); do { sk = sk_next(sk); @@ -3094,10 +3088,7 @@ void udp_seq_stop(struct seq_file *seq, void *v) struct udp_seq_afinfo *afinfo; struct udp_table *udptable; - if (state->bpf_seq_afinfo) - afinfo = state->bpf_seq_afinfo; - else - afinfo = pde_data(file_inode(seq->file)); + afinfo = pde_data(file_inode(seq->file)); udptable = udp_get_table_afinfo(afinfo, seq_file_net(seq)); @@ -3415,28 +3406,11 @@ DEFINE_BPF_ITER_FUNC(udp, struct bpf_iter_meta *meta, static int bpf_iter_init_udp(void *priv_data, struct bpf_iter_aux_info *aux) { - struct udp_iter_state *st = priv_data; - struct udp_seq_afinfo *afinfo; - int ret; - - afinfo = kmalloc(sizeof(*afinfo), GFP_USER | __GFP_NOWARN); - if (!afinfo) - return -ENOMEM; - - afinfo->family = AF_UNSPEC; - afinfo->udp_table = NULL; - st->bpf_seq_afinfo = afinfo; - ret = bpf_iter_init_seq_net(priv_data, aux); - if (ret) - kfree(afinfo); - return ret; + return bpf_iter_init_seq_net(priv_data, aux); } static void bpf_iter_fini_udp(void *priv_data) { - struct udp_iter_state *st = priv_data; - - kfree(st->bpf_seq_afinfo); bpf_iter_fini_seq_net(priv_data); }
This is a preparatory commit to remove the field. The field was previously shared between proc fs and BPF UDP socket iterators. As the follow-up commits will decouple the implementation for the iterators, remove the field. As for BPF socket iterator, filtering of sockets is exepected to be done in BPF programs. Suggested-by: Martin KaFai Lau <martin.lau@kernel.org> Signed-off-by: Aditi Ghag <aditi.ghag@isovalent.com> --- include/net/udp.h | 1 - net/ipv4/udp.c | 34 ++++------------------------------ 2 files changed, 4 insertions(+), 31 deletions(-)