Re: git: 5f11a33ceeb3 - main - if_vlan: do not enable LRO for bridge interaces

I also don’t have a setup to reproduce the issues Paul reported.
Given the discussion in the PR I assumed everyone was happy with the fix he posted.

Best regards,
Kristof

On 12 Aug 2023, at 3:28, Kevin Bowling wrote:
> I think this may be a better fix, currently untested
>
> diff --git a/sys/net/if_vlan.c b/sys/net/if_vlan.c
> index 6aa872a19364..51f0bc63ebda 100644
> --- a/sys/net/if_vlan.c
> +++ b/sys/net/if_vlan.c
> @@ -2074,7 +2074,7 @@ vlan_capabilities(struct ifvlan *ifv)
>        if (p->if_capabilities & IFCAP_VLAN_HWCSUM)
>                cap |= p->if_capabilities & IFCAP_LRO;
>        if (p->if_capenable & IFCAP_VLAN_HWCSUM)
> -               ena |= p->if_capenable & IFCAP_LRO;
> +               ena |= mena & IFCAP_LRO;
>
>        /*
>         * If the parent interface can offload TCP connections over VLANs then
>
> On Fri, Aug 11, 2023 at 5:07 PM Kevin Bowling <kevin.bowling@kev009.com> wrote:
>>
>> I am wondering what is going on with this patch, it doesn't seem like
>> a full fix.  At the very least we should clean up the "can't disable
>> some capabilities" message.
>>
>> I've been looking at this for a couple hours and am wondering what is
>> going on in if_bridge.. it looks like it correctly loops over the
>> interfaces and masks ifcaps it doesn't like using the interface's
>> ioctl SIOCSIFCAP.  A similar pattern is used in if_lagg.  I don't see
>> any issue there so far.  I also don't see the SIOCSICAP ioctl failing
>> nor did vixie's message report that.
>>
>> I checked on a cxgbe(9) bridged vlan setup I have and see this message
>> "bridge0: can't disable some capabilities on vlan0: 0x400"
>>
>> So I am wondering if the bug is that if_vlan SIOCSIFCAP is
>> disregarding the ioctl's request mask.. it is just blindly calling
>> vlan_capabilities.
>>
>> Regards,
>> Kevin
>>
>>
>> On Fri, Aug 11, 2023 at 3:51 PM Kristof Provost <kp@freebsd.org> wrote:
>>>
>>> The branch main has been updated by kp:
>>>
>>> URL: https://cgit.FreeBSD.org/src/commit/?id=5f11a33ceeb385477cb22d9ad5941061c5a26be9
>>>
>>> commit 5f11a33ceeb385477cb22d9ad5941061c5a26be9
>>> Author:     Paul Vixie <paul@redbarn.org>
>>> AuthorDate: 2023-08-11 18:17:16 +0000
>>> Commit:     Kristof Provost <kp@FreeBSD.org>
>>> CommitDate: 2023-08-11 22:50:37 +0000
>>>
>>>     if_vlan: do not enable LRO for bridge interaces
>>>
>>>     If the parent interface is not a bridge and can do LRO and
>>>     checksum offloading on VLANs, then guess it may do LRO on VLANs.
>>>     False positive here cost nothing, while false negative may lead
>>>     to some confusions. According to Wikipedia:
>>>
>>>     "LRO should not operate on machines acting as routers, as it breaks
>>>     the end-to-end principle and can significantly impact performance."
>>>
>>>     The same reasoning applies to machines acting as bridges.
>>>
>>>     PR:             254596
>>>     MFC after:      3 weeks
>>> ---
>>>  sys/net/if_vlan.c | 22 +++++++++++++++-------
>>>  1 file changed, 15 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/sys/net/if_vlan.c b/sys/net/if_vlan.c
>>> index 6aa872a19364..92e4e4247e3d 100644
>>> --- a/sys/net/if_vlan.c
>>> +++ b/sys/net/if_vlan.c
>>> @@ -2067,14 +2067,22 @@ vlan_capabilities(struct ifvlan *ifv)
>>>         }
>>>
>>>         /*
>>> -        * If the parent interface can do LRO and checksum offloading on
>>> -        * VLANs, then guess it may do LRO on VLANs.  False positive here
>>> -        * cost nothing, while false negative may lead to some confusions.
>>> +       * If the parent interface is not a bridge and can do LRO and
>>> +       * checksum offloading on VLANs, then guess it may do LRO on VLANs.
>>> +       * False positive here cost nothing, while false negative may lead
>>> +       * to some confusions. According to Wikipedia:
>>> +       *
>>> +       * "LRO should not operate on machines acting as routers, as it breaks
>>> +       * the end-to-end principle and can significantly impact performance."
>>> +       *
>>> +       * The same reasoning applies to machines acting as bridges.
>>>          */
>>> -       if (p->if_capabilities & IFCAP_VLAN_HWCSUM)
>>> -               cap |= p->if_capabilities & IFCAP_LRO;
>>> -       if (p->if_capenable & IFCAP_VLAN_HWCSUM)
>>> -               ena |= p->if_capenable & IFCAP_LRO;
>>> +       if (ifp->if_bridge == NULL) {
>>> +               if (p->if_capabilities & IFCAP_VLAN_HWCSUM)
>>> +                       cap |= p->if_capabilities & IFCAP_LRO;
>>> +               if (p->if_capenable & IFCAP_VLAN_HWCSUM)
>>> +                       ena |= p->if_capenable & IFCAP_LRO;
>>> +       }
>>>
>>>         /*
>>>          * If the parent interface can offload TCP connections over VLANs then
>>>