lkml.org 
[lkml]   [2013]   [Aug]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2] macvlan: validate flags
On 8/1/2013 12:19 PM, Michael S. Tsirkin wrote:
> On Thu, Aug 01, 2013 at 10:24:19AM -0700, John Fastabend wrote:
>> On 8/1/2013 9:09 AM, Michael S. Tsirkin wrote:
>>> commit df8ef8f3aaa6692970a436204c4429210addb23a
>>> macvlan: add FDB bridge ops and macvlan flags
>>> added a flags field to macvlan, which can be
>>> controlled from userspace.
>>> The idea is to make the interface future-proof
>>> so we can add flags and not new fields.
>>>
>>> However, flags value isn't validated, as a result,
>>> userspace can't detect which flags are supported.
>>>
>>> Cc: "David S. Miller" <davem@davemloft.net>
>>> Cc: John Fastabend <john.r.fastabend@intel.com>
>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>> ---
>>>
>>> Changes from v1:
>>> tweaked commit message
>>> no code changes
>>>
>>> Please consider this patch for -stable.
>>>
>>> The idea is by the time we add more flags,
>>> everyone has updated to a kernel that
>>> detects errors, so userspace will be able
>>> to detect supported flags cleanly.
>>>
>>
>> Agreed and because we haven't added more flags yet this shouldn't
>> break uapi. Thanks for catching this.
>>
>>>
>>> drivers/net/macvlan.c | 7 +++++++
>>> 1 file changed, 7 insertions(+)
>>>
>>
>> By the same logic should we also add the check to macvlan_changelink()?
>
> I'm not sure what do you mean "By the same logic" -
> macvlan_changelink is static unlike macvlan_common_newlink
> which is exported to modules.

"By the same logic" I only meant to allow userspace to cleanly detect
supported flags even in the changelink case.

> So why isn't macvlan_validate sufficient for macvlan_changelink?

It is you are correct.

>
>>> diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
>>> index 18373b6..8445a94 100644
>>> --- a/drivers/net/macvlan.c
>>> +++ b/drivers/net/macvlan.c
>>> @@ -736,6 +736,10 @@ static int macvlan_validate(struct nlattr *tb[], struct nlattr *data[])
>>> return -EADDRNOTAVAIL;
>>> }
>>>
>>> + if (data && data[IFLA_MACVLAN_FLAGS] &&
>>> + nla_get_u16(data[IFLA_MACVLAN_FLAGS]) & ~MACVLAN_FLAG_NOPROMISC)
>>> + return -EINVAL;
>>> +
>>> if (data && data[IFLA_MACVLAN_MODE]) {
>>> switch (nla_get_u32(data[IFLA_MACVLAN_MODE])) {
>>> case MACVLAN_MODE_PRIVATE:
>>> @@ -809,6 +813,9 @@ int macvlan_common_newlink(struct net *src_net, struct net_device *dev,
>>> if (data && data[IFLA_MACVLAN_FLAGS])
>>> vlan->flags = nla_get_u16(data[IFLA_MACVLAN_FLAGS]);
>>>
>>> + if (vlan->flags & ~MACVLAN_FLAG_NOPROMISC)
>>> + return -EINVAL;
>>> +
>>
>> Is there really a case where newlink is called without first calling
>> validate? I don't think there is so the snippet here in newlink could
>> be dropped.
>>
>> Thanks,
>> John
>
> It seems so - macvtap_newlink calls macvlan_common_newlink.
> macvtap does not seem to have .validate.
>

but it calls macvlan_link_register() from macvtap_init which sets
up the validate ops,

int macvlan_link_register(struct rtnl_link_ops *ops)
{
/* common fields */
ops->priv_size = sizeof(struct macvlan_dev);
ops->validate = macvlan_validate


\
 
 \ /
  Last update: 2013-08-01 21:41    [W:0.542 / U:0.100 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site