Messages in this thread |  | | Subject | Re: [PATCH] net: rtnl: avoid uninitialized data in IFLA_VF_VLAN_LIST handling | From | Tariq Toukan <> | Date | Sun, 2 Oct 2016 12:03:37 +0300 |
| |
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) >>> >>> len++; >>> } >>> + 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) >>> -- >>> 2.9.0 >>> Reviewed-by: Moshe Shemesh <moshe@mellanox.com>
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/get_maintainer.pl, but didn't double-check). > I've added them to Cc now, hope they can clarify this. > > Arnd
Regards, Moshe and Tariq
|  |