Re: [PATCH bpf-next v3 2/4] bpf, netns: Keep attached programs in bpf_prog_array

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Jun 25, 2020 at 7:17 AM Jakub Sitnicki <jakub@xxxxxxxxxxxxxx> wrote:
>
> Prepare for having multi-prog attachments for new netns attach types by
> storing programs to run in a bpf_prog_array, which is well suited for
> iterating over programs and running them in sequence.
>
> After this change bpf(PROG_QUERY) may block to allocate memory in
> bpf_prog_array_copy_to_user() for collected program IDs. This forces a
> change in how we protect access to the attached program in the query
> callback. Because bpf_prog_array_copy_to_user() can sleep, we switch from
> an RCU read lock to holding a mutex that serializes updaters.
>
> Because we allow only one BPF flow_dissector program to be attached to
> netns at all times, the bpf_prog_array pointed by net->bpf.run_array is
> always either detached (null) or one element long.
>
> No functional changes intended.
>
> Signed-off-by: Jakub Sitnicki <jakub@xxxxxxxxxxxxxx>
> ---

I wonder if instead of NULL prog_array, it's better to just use a
dummy empty (but allocated) array. Might help eliminate some of the
IFs, maybe even in the hot path.


>  include/net/netns/bpf.h    |   5 +-
>  kernel/bpf/net_namespace.c | 120 +++++++++++++++++++++++++------------
>  net/core/flow_dissector.c  |  19 +++---
>  3 files changed, 96 insertions(+), 48 deletions(-)
>

[...]


>
> +/* Must be called with netns_bpf_mutex held. */
> +static int __netns_bpf_prog_query(const union bpf_attr *attr,
> +                                 union bpf_attr __user *uattr,
> +                                 struct net *net,
> +                                 enum netns_bpf_attach_type type)
> +{
> +       __u32 __user *prog_ids = u64_to_user_ptr(attr->query.prog_ids);
> +       struct bpf_prog_array *run_array;
> +       u32 prog_cnt = 0, flags = 0;
> +
> +       run_array = rcu_dereference_protected(net->bpf.run_array[type],
> +                                             lockdep_is_held(&netns_bpf_mutex));
> +       if (run_array)
> +               prog_cnt = bpf_prog_array_length(run_array);
> +
> +       if (copy_to_user(&uattr->query.attach_flags, &flags, sizeof(flags)))
> +               return -EFAULT;
> +       if (copy_to_user(&uattr->query.prog_cnt, &prog_cnt, sizeof(prog_cnt)))
> +               return -EFAULT;
> +       if (!attr->query.prog_cnt || !prog_ids || !prog_cnt)
> +               return 0;
> +
> +       return bpf_prog_array_copy_to_user(run_array, prog_ids,
> +                                          attr->query.prog_cnt);

It doesn't seem like bpf_prog_array_copy_to_user can handle NULL run_array

> +}
> +
>  int netns_bpf_prog_query(const union bpf_attr *attr,
>                          union bpf_attr __user *uattr)
>  {
> -       __u32 __user *prog_ids = u64_to_user_ptr(attr->query.prog_ids);
> -       u32 prog_id, prog_cnt = 0, flags = 0;
>         enum netns_bpf_attach_type type;
> -       struct bpf_prog *attached;
>         struct net *net;
> +       int ret;
>
>         if (attr->query.query_flags)
>                 return -EINVAL;

[...]



[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux