netdev - Re: [RFC net-next 2/9] net: bridge: Disambiguate offload_fwd

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 03 May 2021 10:49:12 +0200
From:   Tobias Waldekranz <tobias@...dekranz.com>
To:     Ido Schimmel <idosch@...sch.org>
Cc:     davem@...emloft.net, kuba@...nel.org, andrew@...n.ch,
        vivien.didelot@...il.com, f.fainelli@...il.com, olteanv@...il.com,
        roopa@...dia.com, nikolay@...dia.com, jiri@...nulli.us,
        stephen@...workplumber.org, netdev@...r.kernel.org,
        bridge@...ts.linux-foundation.org
Subject: Re: [RFC net-next 2/9] net: bridge: Disambiguate offload_fwd_mark

On Sun, May 02, 2021 at 18:00, Ido Schimmel <idosch@...sch.org> wrote:
> On Mon, Apr 26, 2021 at 07:04:04PM +0200, Tobias Waldekranz wrote:
>> - skb->cb->offload_fwd_mark becomes skb->cb->src_hwdom. There is a
>>   slight change here: Whereas previously this was only set for
>>   offloaded packets, we now always track the incoming hwdom. As all
>>   uses where already gated behind checks of skb->offload_fwd_mark,
>>   this will not introduce any functional change, but it paves the way
>>   for future changes where the ingressing hwdom must be known both for
>>   offloaded and non-offloaded frames.
>
> [...]
>
>> @@ -43,15 +43,15 @@ int nbp_switchdev_mark_set(struct net_bridge_port *p)
>>  void nbp_switchdev_frame_mark(const struct net_bridge_port *p,
>>  			      struct sk_buff *skb)
>>  {
>> -	if (skb->offload_fwd_mark && !WARN_ON_ONCE(!p->offload_fwd_mark))
>> -		BR_INPUT_SKB_CB(skb)->offload_fwd_mark = p->offload_fwd_mark;
>> +	if (p->hwdom)
>> +		BR_INPUT_SKB_CB(skb)->src_hwdom = p->hwdom;
>>  }
>
> I assume you are referring to this change? "src_hwdom" sounds weird if
> it's expected to be valid for non-offloaded frames.

Perhaps "non-offloaded" was a sloppy description on my part. I was
trying to describe frames that originate from a switchdev, but have not
been forwarded by hardware; e.g. STP BPDUs, IGMP reports, etc. So
nbp_switchdev_frame_mark now basically says: "If this skb came in from a
switchdev, make sure to note which one".

> Can you elaborate about "future changes where the ingressing hwdom must
> be known both for offloaded and non-offloaded frames"?

Typical example: The switchdev has a fixed configuration to trap STP
BPDUs, but STP is not running on the bridge and the group_fwd_mask
allows them to be forwarded. Say we have this setup:

      br0
    /  |  \
swp0 swp1 swp2

A BPDU comes in on swp0 and is trapped to the CPU; the driver does not
set skb->offload_fwd_mark. The bridge determines that the frame should
be forwarded to swp{1,2}. It is imperative that forward offloading is
_not_ allowed in this case, as the source hwdom is already "poisoned".

Recording the source hwdom allows this case to be handled properly.

> Probably best to split this change to a different patch given the rest
> of the changes are mechanical.

Right, but I think the change in name to warrants a change in
semantics. It is being renamed to src_hwdom because it now holds just
that information. Again, there is no functional change introduced by
this since nbp_switchdev_allowed_egress always checks for the presence
of skb->offload_fwd_mark anyway. But if you feel strongly about it, I
will split it up.

>>  
>>  bool nbp_switchdev_allowed_egress(const struct net_bridge_port *p,
>>  				  const struct sk_buff *skb)
>>  {
>>  	return !skb->offload_fwd_mark ||
>> -	       BR_INPUT_SKB_CB(skb)->offload_fwd_mark != p->offload_fwd_mark;
>> +	       BR_INPUT_SKB_CB(skb)->src_hwdom != p->hwdom;
>>  }
>>  
>>  /* Flags that can be offloaded to hardware */
>> -- 
>> 2.25.1
>>