Re: [PATCH] net: rtnl: avoid uninitialized data in IFLA_VF_VLAN_LIST handling

From: Tariq Toukan
Date: Sun Oct 02 2016 - 15:36:52 EST

Hi Arnd,

On 30/09/2016 7:38 PM, Arnd Bergmann wrote:
On Friday 30 September 2016, Eric Dumazet wrote:
@@ -1753,6 +1753,9 @@ static int do_setvfinfo(struct net_device *dev, struct nlattr **tb)

+ if (len == 0)
+ return -EINVAL;
err = ops->ndo_set_vf_vlan(dev, ivvl[0]->vf, ivvl[0]->vlan,
ivvl[0]->qos, ivvl[0]->vlan_proto);
if (err < 0)

Reviewed-by: Moshe Shemesh <moshe@xxxxxxxxxxxx>

Thanks for the fix, I will add the flag to my makefile.
So, if I read this code, we build an array, but call ndo_set_vf_vlan()
only using first element ?

Looks like the bug should be fixed in a different way.
I was wondering about this too, but didn't understand enough about it to say
if it was intentional or not.
It is intentional.
The change we need here is the option to set the VF vlan protocol, so our hw will insert either S-TAG or C-TAG per VF. The previous version of this patch didn't use any array.
Then we got a comment, which makes sense, that the new UAPI interface should be able to support a future driver that might want to set both S-TAG and C-TAG per VF.
We could have used a single struct on kernel side instead of a list of size 1, but this would cause bad/not scalable code in the parsing stage.
As the UAPI is a list, in the parsing stage we thought it is more correct to use the constant MAX_VLAN_LIST_LEN and iterate as of a general case, even though currently the list size is one.

I just realized that I forgot to add Moshe and Tariq
on Cc (I relied on scripts/, but didn't double-check).
I've added them to Cc now, hope they can clarify this.


Moshe and Tariq