On Tue, Oct 4, 2022 at 4:12 PM Daniel Borkmann <daniel@xxxxxxxxxxxxx> wrote:
>
> Add support for LINK_UPDATE command for tc BPF link to allow for a reliable
> replacement of the underlying BPF program.
>
> Co-developed-by: Nikolay Aleksandrov <razor@xxxxxxxxxxxxx>
> Signed-off-by: Nikolay Aleksandrov <razor@xxxxxxxxxxxxx>
> Signed-off-by: Daniel Borkmann <daniel@xxxxxxxxxxxxx>
> ---
> kernel/bpf/net.c | 34 ++++++++++++++++++++++++++++++++++
> 1 file changed, 34 insertions(+)
>
> diff --git a/kernel/bpf/net.c b/kernel/bpf/net.c
> index 22b7a9b05483..c50bcf656b3f 100644
> --- a/kernel/bpf/net.c
> +++ b/kernel/bpf/net.c
> @@ -303,6 +303,39 @@ static int __xtc_link_attach(struct bpf_link *l, u32 id)
> return ret;
> }
>
> +static int xtc_link_update(struct bpf_link *l, struct bpf_prog *nprog,
> + struct bpf_prog *oprog)
> +{
> + struct bpf_tc_link *link = container_of(l, struct bpf_tc_link, link);
> + int ret = 0;
> +
> + rtnl_lock();
> + if (!link->dev) {
> + ret = -ENOLINK;
> + goto out;
> + }
> + if (oprog && l->prog != oprog) {
> + ret = -EPERM;
> + goto out;
> + }
> + oprog = l->prog;
> + if (oprog == nprog) {
> + bpf_prog_put(nprog);
> + goto out;
> + }
> + ret = __xtc_prog_attach(link->dev, link->location == BPF_NET_INGRESS,
> + XTC_MAX_ENTRIES, l->id, nprog, link->priority,
> + BPF_F_REPLACE);
> + if (ret == link->priority) {
prog_attach returning priority is quite confusing. I think it's
because we support specifying zero and letting kernel pick priority,
so we need to communicate it back, is that right? If yes, can you
please add comment to xtc_prog_attach explaining this behavior?
and also, here if it's not an error then priority *has* to be equal to
link->priority, right? So:
if (ret < 0)
goto out;
oprog = xchg(...)
bpf_prog_put(...)
ret = 0;
would be easier to follow, otherwise we are left wondering what
happens when ret > 0 && ret != link->priority. If you are worried of
bugs, BUG_ON/WARN_ON if ret != link->priority?
> + oprog = xchg(&l->prog, nprog);
> + bpf_prog_put(oprog);
> + ret = 0;
> + }
> +out:
> + rtnl_unlock();
> + return ret;
> +}
> +
> static void xtc_link_release(struct bpf_link *l)
> {
> struct bpf_tc_link *link = container_of(l, struct bpf_tc_link, link);
> @@ -327,6 +360,7 @@ static void xtc_link_dealloc(struct bpf_link *l)
> static const struct bpf_link_ops bpf_tc_link_lops = {
> .release = xtc_link_release,
> .dealloc = xtc_link_dealloc,
> + .update_prog = xtc_link_update,
> };
>
> int xtc_link_attach(const union bpf_attr *attr, struct bpf_prog *prog)
> --
> 2.34.1
>
