> On Apr 23, 2023, at 5:18 PM, Martin KaFai Lau <martin.lau@xxxxxxxxx> 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@xxxxxxxxxx>
>> Signed-off-by: Aditi Ghag <aditi.ghag@xxxxxxxxxxxxx>
>> ---
>> 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);
>> }
