netdev - Re: [PATCH net-next 1/2] net: vlan: Replace BUG() with WARN_ON_ONCE() in vlan_dev

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <611fe2fd-d826-4ffb-9f5b-5eb7ba99ae62@nvidia.com>
Date: Thu, 12 Jun 2025 13:32:33 +0300
From: Gal Pressman <gal@...dia.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: "David S. Miller" <davem@...emloft.net>,
 Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>,
 Andrew Lunn <andrew+netdev@...n.ch>, netdev@...r.kernel.org,
 Alex Lazar <alazar@...dia.com>, Dragos Tatulea <dtatulea@...dia.com>
Subject: Re: [PATCH net-next 1/2] net: vlan: Replace BUG() with WARN_ON_ONCE()
 in vlan_dev_* stubs

On 12/06/2025 3:15, Jakub Kicinski wrote:
> On Tue, 10 Jun 2025 10:26:10 +0300 Gal Pressman wrote:
>> This code should not be reached, as callers of these functions should
>> always check for is_vlan_dev() first, but the usage of BUG() is not
>> recommended, replace it with WARN_ON() instead.
> 
> Did you try something along the lines of:
> 
> diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
> index 38456b42cdb5..f24af2988fd9 100644
> --- a/include/linux/if_vlan.h
> +++ b/include/linux/if_vlan.h
> @@ -81,7 +81,8 @@ extern void vlan_ioctl_set(int (*hook)(struct net *, void __user *));
>  
>  static inline bool is_vlan_dev(const struct net_device *dev)
>  {
> -        return dev->priv_flags & IFF_802_1Q_VLAN;
> +        return IS_ENABLED(CONFIG_VLAN_8021Q) &&
> +               dev->priv_flags & IFF_802_1Q_VLAN;
>  }
>  
>  #define skb_vlan_tag_present(__skb)    (!!(__skb)->vlan_all)
> 
> 
> This should let the compiler eliminate the dead code completely,
> assuming it truly dead. We can still replace the BUG()s as 
> a of cleanup but I think the above is strictly preferable as 
> a solution to your problem?

Makes sense.

If we're going in this direction, maybe just provide a stub
is_vlan_dev() for the !IS_ENABLED case?