Re: [PATCH net-next v2 1/7] net: ip: make fib_validate_source() return drop reason

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

 



On 10/11/24 08:42, Menglong Dong wrote:
On Thu, Oct 10, 2024 at 5:18 PM Menglong Dong <menglong8.dong@xxxxxxxxx> wrote:
On Thu, Oct 10, 2024 at 4:25 PM Paolo Abeni <pabeni@xxxxxxxxxx> wrote:
On 10/7/24 09:46, Menglong Dong wrote:
In this commit, we make fib_validate_source/__fib_validate_source return
-reason instead of errno on error. As the return value of them can be
-errno, 0, and 1, we can't make it return enum skb_drop_reason directly.

In the origin logic, if __fib_validate_source() return -EXDEV,
LINUX_MIB_IPRPFILTER will be counted. And now, we need to adjust it by
checking "reason == SKB_DROP_REASON_IP_RPFILTER". However, this will take
effect only after the patch "net: ip: make ip_route_input_noref() return
drop reasons", as we can't pass the drop reasons from
fib_validate_source() to ip_rcv_finish_core() in this patch.

We set the errno to -EINVAL when fib_validate_source() is called and the
validation fails, as the errno can be checked in the caller and now its
value is -reason, which can lead misunderstand.

Following new drop reasons are added in this patch:

    SKB_DROP_REASON_IP_LOCAL_SOURCE
    SKB_DROP_REASON_IP_INVALID_SOURCE

Signed-off-by: Menglong Dong <dongml2@xxxxxxxxxxxxxxx>

Looking at the next patches, I'm under the impression that the overall
code will be simpler if you let __fib_validate_source() return directly
a drop reason, and fib_validate_source(), too. Hard to be sure without
actually do the attempt... did you try such patch by any chance?


I analysed the usages of fib_validate_source() before. The
return value of fib_validate_source() can be -errno, "0", and "1".
And the value "1" can be used by the caller, such as
__mkroute_input(). Making it return drop reasons can't cover this
case.

It seems that __mkroute_input() is the only case that uses the
positive returning value of fib_validate_source(). Let me think
about it more in this case.

Hello,

After digging into the code of __fib_validate_source() and __mkroute_input(),
I think it's hard to make __fib_validate_source() return drop reasons
directly.

The __fib_validate_source() will return 1 if the scope of the
source(revert) route is HOST. And the __mkroute_input()
will mark the skb with IPSKB_DOREDIRECT in this
case (combine with some other conditions). And then, a REDIRECT
ICMP will be sent in ip_forward() if this flag exists.

I don't find a way to pass this information to __mkroute_input
if we make __fib_validate_source() return drop reasons. Can we?

An option is to add a wrapper for fib_validate_source(), such as
fib_validate_source_reason(), which returns drop reasons. And in
__mkroute_input(), we still call fib_validate_source().

What do you think?

Thanks for the investigation. I see that let __fib_validate_source() returning drop reasons does not look like a good design.

I think the additional helper will not help much, so I guess you can retain the current implementation here, but please expand the commit message with the above information.

Thanks!

Paolo





[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