All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net] macvlan: don't touch promisc without passthrough
@ 2013-06-12 11:34 Michael S. Tsirkin
  2013-06-12 13:56 ` Sergei Shtylyov
  0 siblings, 1 reply; 3+ messages in thread
From: Michael S. Tsirkin @ 2013-06-12 11:34 UTC (permalink / raw)
  To: linux-kernel
  Cc: David S. Miller, Roopa Prabhu, John Fastabend, Patrick McHardy, netdev

commit df8ef8f3aaa6692970a436204c4429210addb23a in linux 3.5 added a way
to control NOPROMISC macvlan flag through netlink.

However, with a non passthrough device we don't set promisc on open or
clear it on stop, even if NOPROMISC is off.  As a result:

If userspace clears NOPROMISC on open, then does not clear it on a
netlink command, promisc counter is not decremented on stop and there
will be no way to clear it once macvlan is detached.

If userspace does not clear NOPROMISC on open, then sets NOPROMISC on a
netlink command, promisc counter will be decremented from 0 and overflow
to fffffffff with no way to clear promisc afterwards.

To fix, simply ignore NOPROMISC flag in a netlink command for
non-passthrough devices, same as we do at open/stop.

While at it - since we touch this code anyway - check
dev_set_promiscuity return code and pass it to users (though an error
here is unlikely).

Cc: "David S. Miller" <davem@davemloft.net>
CC: Roopa Prabhu <roprabhu@cisco.com>
Cc: John Fastabend <john.r.fastabend@intel.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---

Please review, and consider for 3.10 and -stable.

 drivers/net/macvlan.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index 1c502bb..8812402 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -853,18 +853,23 @@ static int macvlan_changelink(struct net_device *dev,
 		struct nlattr *tb[], struct nlattr *data[])
 {
 	struct macvlan_dev *vlan = netdev_priv(dev);
-	if (data && data[IFLA_MACVLAN_MODE])
-		vlan->mode = nla_get_u32(data[IFLA_MACVLAN_MODE]);
+
 	if (data && data[IFLA_MACVLAN_FLAGS]) {
 		__u16 flags = nla_get_u16(data[IFLA_MACVLAN_FLAGS]);
 		bool promisc = (flags ^ vlan->flags) & MACVLAN_FLAG_NOPROMISC;
-
-		if (promisc && (flags & MACVLAN_FLAG_NOPROMISC))
-			dev_set_promiscuity(vlan->lowerdev, -1);
-		else if (promisc && !(flags & MACVLAN_FLAG_NOPROMISC))
-			dev_set_promiscuity(vlan->lowerdev, 1);
+		if (vlan->port->passthru && promisc) {
+			int err;
+			if (flags & MACVLAN_FLAG_NOPROMISC)
+				err = dev_set_promiscuity(vlan->lowerdev, -1);
+			else
+				err = dev_set_promiscuity(vlan->lowerdev, 1);
+			if (err < 0)
+				return err;
+		}
 		vlan->flags = flags;
 	}
+	if (data && data[IFLA_MACVLAN_MODE])
+		vlan->mode = nla_get_u32(data[IFLA_MACVLAN_MODE]);
 	return 0;
 }
 
-- 
MST

^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH net] macvlan: don't touch promisc without passthrough
  2013-06-12 11:34 [PATCH net] macvlan: don't touch promisc without passthrough Michael S. Tsirkin
@ 2013-06-12 13:56 ` Sergei Shtylyov
  2013-06-13  6:02   ` John Fastabend
  0 siblings, 1 reply; 3+ messages in thread
From: Sergei Shtylyov @ 2013-06-12 13:56 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: linux-kernel, David S. Miller, Roopa Prabhu, John Fastabend,
	Patrick McHardy, netdev

Hello.

On 12-06-2013 15:34, Michael S. Tsirkin wrote:

> commit df8ef8f3aaa6692970a436204c4429210addb23a in linux 3.5 added a way

    Please also specify that commit's summary line in parens.

> to control NOPROMISC macvlan flag through netlink.

> However, with a non passthrough device we don't set promisc on open or
> clear it on stop, even if NOPROMISC is off.  As a result:

> If userspace clears NOPROMISC on open, then does not clear it on a
> netlink command, promisc counter is not decremented on stop and there
> will be no way to clear it once macvlan is detached.

> If userspace does not clear NOPROMISC on open, then sets NOPROMISC on a
> netlink command, promisc counter will be decremented from 0 and overflow
> to fffffffff with no way to clear promisc afterwards.

> To fix, simply ignore NOPROMISC flag in a netlink command for
> non-passthrough devices, same as we do at open/stop.

> While at it - since we touch this code anyway - check
> dev_set_promiscuity return code and pass it to users (though an error
> here is unlikely).

> Cc: "David S. Miller" <davem@davemloft.net>
> CC: Roopa Prabhu <roprabhu@cisco.com>
> Cc: John Fastabend <john.r.fastabend@intel.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---

> Please review, and consider for 3.10 and -stable.

>   drivers/net/macvlan.c | 19 ++++++++++++-------
>   1 file changed, 12 insertions(+), 7 deletions(-)

> diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
> index 1c502bb..8812402 100644
> --- a/drivers/net/macvlan.c
> +++ b/drivers/net/macvlan.c
> @@ -853,18 +853,23 @@ static int macvlan_changelink(struct net_device *dev,
>   		struct nlattr *tb[], struct nlattr *data[])
>   {
>   	struct macvlan_dev *vlan = netdev_priv(dev);
> -	if (data && data[IFLA_MACVLAN_MODE])
> -		vlan->mode = nla_get_u32(data[IFLA_MACVLAN_MODE]);
> +
>   	if (data && data[IFLA_MACVLAN_FLAGS]) {
>   		__u16 flags = nla_get_u16(data[IFLA_MACVLAN_FLAGS]);
>   		bool promisc = (flags ^ vlan->flags) & MACVLAN_FLAG_NOPROMISC;
> -
> -		if (promisc && (flags & MACVLAN_FLAG_NOPROMISC))
> -			dev_set_promiscuity(vlan->lowerdev, -1);
> -		else if (promisc && !(flags & MACVLAN_FLAG_NOPROMISC))
> -			dev_set_promiscuity(vlan->lowerdev, 1);
> +		if (vlan->port->passthru && promisc) {
> +			int err;

    Emoty line, like you did abobe, wouldn't hurt here, after declaration.

> +			if (flags & MACVLAN_FLAG_NOPROMISC)
> +				err = dev_set_promiscuity(vlan->lowerdev, -1);
> +			else
> +				err = dev_set_promiscuity(vlan->lowerdev, 1);
> +			if (err < 0)
> +				return err;
> +		}

WBR, Sergei


^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [PATCH net] macvlan: don't touch promisc without passthrough
  2013-06-12 13:56 ` Sergei Shtylyov
@ 2013-06-13  6:02   ` John Fastabend
  0 siblings, 0 replies; 3+ messages in thread
From: John Fastabend @ 2013-06-13  6:02 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: Sergei Shtylyov, linux-kernel, David S. Miller, Roopa Prabhu,
	John Fastabend, Patrick McHardy, netdev

On 06/12/2013 06:56 AM, Sergei Shtylyov wrote:
> Hello.
>
> On 12-06-2013 15:34, Michael S. Tsirkin wrote:
>
>> commit df8ef8f3aaa6692970a436204c4429210addb23a in linux 3.5 added a way
>
>     Please also specify that commit's summary line in parens.
>
>> to control NOPROMISC macvlan flag through netlink.
>
>> However, with a non passthrough device we don't set promisc on open or
>> clear it on stop, even if NOPROMISC is off.  As a result:
>
>> If userspace clears NOPROMISC on open, then does not clear it on a
>> netlink command, promisc counter is not decremented on stop and there
>> will be no way to clear it once macvlan is detached.
>
>> If userspace does not clear NOPROMISC on open, then sets NOPROMISC on a
>> netlink command, promisc counter will be decremented from 0 and overflow
>> to fffffffff with no way to clear promisc afterwards.
>
>> To fix, simply ignore NOPROMISC flag in a netlink command for
>> non-passthrough devices, same as we do at open/stop.
>
>> While at it - since we touch this code anyway - check
>> dev_set_promiscuity return code and pass it to users (though an error
>> here is unlikely).
>
>> Cc: "David S. Miller" <davem@davemloft.net>
>> CC: Roopa Prabhu <roprabhu@cisco.com>
>> Cc: John Fastabend <john.r.fastabend@intel.com>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> ---
>
>> Please review, and consider for 3.10 and -stable.
>

Other than those few nits looks good to me thanks!

Reviewed-by: John Fastabend <john.r.fastabend@intel.com>

-- 
John Fastabend         Intel Corporation

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2013-06-13  6:02 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-06-12 11:34 [PATCH net] macvlan: don't touch promisc without passthrough Michael S. Tsirkin
2013-06-12 13:56 ` Sergei Shtylyov
2013-06-13  6:02   ` John Fastabend

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.