netdev - Re: [PATCH net-next] net: vlan: Avoid using BUG() in vlan_proto

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: <20200924164615.619badc0@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Thu, 24 Sep 2020 16:46:15 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Florian Fainelli <f.fainelli@...il.com>
Cc:     netdev@...r.kernel.org, olteanv@...il.com, vladimir.oltean@....com
Subject: Re: [PATCH net-next] net: vlan: Avoid using BUG() in
 vlan_proto_idx()

On Wed, 23 Sep 2020 21:16:27 -0700 Florian Fainelli wrote:
> While we should always make sure that we specify a valid VLAN protocol
> to vlan_proto_idx(), killing the machine when an invalid value is
> specified is too harsh and not helpful for debugging. All callers are
> capable of dealing with an error returned by vlan_proto_idx() so check
> the index value and propagate it accordingly.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@...il.com>

Perhaps it's heresy but I wonder if the error checking is worth it 
or we'd be better off WARNing and assuming normal Q tag.. unlikely
someone is getting it wrong now given the BUG().

> diff --git a/net/8021q/vlan.c b/net/8021q/vlan.c
> index d4bcfd8f95bf..6c08de1116c1 100644
> --- a/net/8021q/vlan.c
> +++ b/net/8021q/vlan.c
> @@ -57,6 +57,9 @@ static int vlan_group_prealloc_vid(struct vlan_group *vg,
>  	ASSERT_RTNL();
>  
>  	pidx  = vlan_proto_idx(vlan_proto);
> +	if (pidx < 0)
> +		return -EINVAL;
> +
>  	vidx  = vlan_id / VLAN_GROUP_ARRAY_PART_LEN;
>  	array = vg->vlan_devices_arrays[pidx][vidx];
>  	if (array != NULL)
> diff --git a/net/8021q/vlan.h b/net/8021q/vlan.h
> index bb7ec1a3915d..143e9c12dbd6 100644
> --- a/net/8021q/vlan.h
> +++ b/net/8021q/vlan.h
> @@ -44,8 +44,8 @@ static inline unsigned int vlan_proto_idx(__be16 proto)

adjust return type

>  	case htons(ETH_P_8021AD):
>  		return VLAN_PROTO_8021AD;
>  	default:
> -		BUG();
> -		return 0;
> +		WARN(1, "invalid VLAN protocol: 0x%04x\n", htons(proto));

ntohs()

> +		return -EINVAL;
>  	}
>  }