Avinash Duduskar <[email protected]> writes:
> Toke Høiland-Jørgensen <[email protected]> writes:
>
>> I think it's better to just move the assignment of params->ifindex
>> entirely into bpf_fib_set_fwd_params(), instead of this restore dance.
>> That way this can be simplified to:
>>
>> err = bpf_fib_set_fwd_params(dev, params, flags, mtu);
>> if (!err && fwd_dev)
>> *fwd_dev = dev;
>> return err;
>
> The caller-side restore is ungainly, agreed, but the assignment can't move
> all the way into the helper. The early params->ifindex = dev->ifindex
> sits above the neighbour lookup on purpose: that is d1c362e1dd68a
> ("bpf: Always return target ifindex in bpf_fib_lookup"), which took it
> out of bpf_fib_set_fwd_params() and put it there so a program still
> gets the target ifindex on the BPF_FIB_LKUP_RET_NO_NEIGH path and can
> bpf_redirect_neigh() on it. bpf_fib_set_fwd_params() is called only at
> the set_fwd_params label, below the NO_NEIGH return (and below the IPv6
> NO_SRC_ADDR return), so an assignment living in the helper never runs
> on those paths and params->ifindex falls back to the input. That would
> change the reported ifindex for plain bpf_fib_lookup() callers hitting
> NO_NEIGH, not only the VLAN ones.
Right. Well, seems I forgot about that patch, even though I seem to have
written it :)
> I can still get the caller down to your form by keeping the early write
> and moving just the VLAN_FAILURE rewind into the helper, with one extra
> parameter, the input ifindex saved before the egress write:
>
> err = bpf_fib_set_fwd_params(dev, params, flags, mtu, in_ifindex);
> if (!err && fwd_dev)
> *fwd_dev = dev;
> return err;
>
> and the helper owning the rewind in the unreducible branch:
>
> } else {
> params->ifindex = in_ifindex;
> return BPF_FIB_LKUP_RET_VLAN_FAILURE;
> }
OK, if we do need to restore it, I think it's better to do it there.
Also, wrt the fwd_dev parameter: Do we really have a use case from using
this from TC? In TC you can just redirect to the VLAN device; this is
meant for XDP which can't do that. So how about we just reject the flag
on the TC side, and get rid of the fwd_dev parameter entirely?
If we do that we're back to just a plain 'return bpf_fib_set_fwd_params()' :)
-Toke
