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" <[email protected]>
Cc: John Fastabend <[email protected]>
Signed-off-by: Michael S. Tsirkin <[email protected]>
---
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.
drivers/net/macvlan.c | 7 +++++++
1 file changed, 7 insertions(+)
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;
+
if (vlan->mode == MACVLAN_MODE_PASSTHRU) {
if (port->count)
return -EINVAL;
--
MST
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" <[email protected]>
> Cc: John Fastabend <[email protected]>
> Signed-off-by: Michael S. Tsirkin <[email protected]>
> ---
>
> 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()?
> 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
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" <[email protected]>
> >Cc: John Fastabend <[email protected]>
> >Signed-off-by: Michael S. Tsirkin <[email protected]>
> >---
> >
> >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.
So why isn't macvlan_validate sufficient for macvlan_changelink?
> >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.
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" <[email protected]>
>>> Cc: John Fastabend <[email protected]>
>>> Signed-off-by: Michael S. Tsirkin <[email protected]>
>>> ---
>>>
>>> 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
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" <[email protected]>
>>> Cc: John Fastabend <[email protected]>
>>> Signed-off-by: Michael S. Tsirkin <[email protected]>
>>> ---
>>>
>>> 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
On Thu, Aug 01, 2013 at 12:34:24PM -0700, John Fastabend wrote:
> 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" <[email protected]>
> >>>Cc: John Fastabend <[email protected]>
> >>>Signed-off-by: Michael S. Tsirkin <[email protected]>
> >>>---
> >>>
> >>>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
I see. I'll drop this second chunk in the patch,
thanks for catching this.
--
MST