* [PATCHv3] net: core: Always propagate flag changes to interfaces
@ 2013-11-20 1:47 Vlad Yasevich
2013-11-20 7:21 ` Stefan Priebe - Profihost AG
2013-11-20 20:30 ` David Miller
0 siblings, 2 replies; 5+ messages in thread
From: Vlad Yasevich @ 2013-11-20 1:47 UTC (permalink / raw)
To: netdev; +Cc: s.priebe, vfalico, Vlad Yasevich
The following commit:
b6c40d68ff6498b7f63ddf97cf0aa818d748dee7
net: only invoke dev->change_rx_flags when device is UP
tried to fix a problem with VLAN devices and promiscuouse flag setting.
The issue was that VLAN device was setting a flag on an interface that
was down, thus resulting in bad promiscuity count.
This commit blocked flag propagation to any device that is currently
down.
A later commit:
deede2fabe24e00bd7e246eb81cd5767dc6fcfc7
vlan: Don't propagate flag changes on down interfaces
fixed VLAN code to only propagate flags when the VLAN interface is up,
thus fixing the same issue as above, only localized to VLAN.
The problem we have now is that if we have create a complex stack
involving multiple software devices like bridges, bonds, and vlans,
then it is possible that the flags would not propagate properly to
the physical devices. A simple examle of the scenario is the
following:
eth0----> bond0 ----> bridge0 ---> vlan50
If bond0 or eth0 happen to be down at the time bond0 is added to
the bridge, then eth0 will never have promisc mode set which is
currently required for operation as part of the bridge. As a
result, packets with vlan50 will be dropped by the interface.
The only 2 devices that implement the special flag handling are
VLAN and DSA and they both have required code to prevent incorrect
flag propagation. As a result we can remove the generic solution
introduced in b6c40d68ff6498b7f63ddf97cf0aa818d748dee7 and leave
it to the individual devices to decide whether they will block
flag propagation or not.
Reported-by: Stefan Priebe <s.priebe@profihost.ag>
Suggested-by: Veaceslav Falico <vfalico@redhat.com>
Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
---
v2->v3: Removed a strange chunk that modified comments. Not sure where it
came from.
net/core/dev.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/core/dev.c b/net/core/dev.c
index 974143d..da9c5e1 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -4991,7 +4991,7 @@ static void dev_change_rx_flags(struct net_device *dev, int flags)
{
const struct net_device_ops *ops = dev->netdev_ops;
- if ((dev->flags & IFF_UP) && ops->ndo_change_rx_flags)
+ if (ops->ndo_change_rx_flags)
ops->ndo_change_rx_flags(dev, flags);
}
--
1.8.4.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCHv3] net: core: Always propagate flag changes to interfaces
2013-11-20 1:47 [PATCHv3] net: core: Always propagate flag changes to interfaces Vlad Yasevich
@ 2013-11-20 7:21 ` Stefan Priebe - Profihost AG
2013-11-20 13:54 ` Vlad Yasevich
2013-11-20 20:30 ` David Miller
1 sibling, 1 reply; 5+ messages in thread
From: Stefan Priebe - Profihost AG @ 2013-11-20 7:21 UTC (permalink / raw)
To: Vlad Yasevich, netdev; +Cc: vfalico
Hi,
Am 20.11.2013 02:47, schrieb Vlad Yasevich:
> The following commit:
> b6c40d68ff6498b7f63ddf97cf0aa818d748dee7
> net: only invoke dev->change_rx_flags when device is UP
>
> tried to fix a problem with VLAN devices and promiscuouse flag setting.
> The issue was that VLAN device was setting a flag on an interface that
> was down, thus resulting in bad promiscuity count.
> This commit blocked flag propagation to any device that is currently
> down.
>
> A later commit:
> deede2fabe24e00bd7e246eb81cd5767dc6fcfc7
> vlan: Don't propagate flag changes on down interfaces
>
> fixed VLAN code to only propagate flags when the VLAN interface is up,
> thus fixing the same issue as above, only localized to VLAN.
>
> The problem we have now is that if we have create a complex stack
> involving multiple software devices like bridges, bonds, and vlans,
> then it is possible that the flags would not propagate properly to
> the physical devices. A simple examle of the scenario is the
> following:
>
> eth0----> bond0 ----> bridge0 ---> vlan50
>
> If bond0 or eth0 happen to be down at the time bond0 is added to
> the bridge, then eth0 will never have promisc mode set which is
> currently required for operation as part of the bridge. As a
> result, packets with vlan50 will be dropped by the interface.
>
> The only 2 devices that implement the special flag handling are
> VLAN and DSA and they both have required code to prevent incorrect
> flag propagation. As a result we can remove the generic solution
> introduced in b6c40d68ff6498b7f63ddf97cf0aa818d748dee7 and leave
> it to the individual devices to decide whether they will block
> flag propagation or not.
>
> Reported-by: Stefan Priebe <s.priebe@profihost.ag>
> Suggested-by: Veaceslav Falico <vfalico@redhat.com>
> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
> ---
> v2->v3: Removed a strange chunk that modified comments. Not sure where it
> came from.
>
> net/core/dev.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 974143d..da9c5e1 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -4991,7 +4991,7 @@ static void dev_change_rx_flags(struct net_device *dev, int flags)
> {
> const struct net_device_ops *ops = dev->netdev_ops;
>
> - if ((dev->flags & IFF_UP) && ops->ndo_change_rx_flags)
> + if (ops->ndo_change_rx_flags)
> ops->ndo_change_rx_flags(dev, flags);
> }
thanks for this patch - in one of the first posts you send this one:
diff --git a/net/core/dev.c b/net/core/dev.c
index fc913f4..016857b 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -4525,7 +4525,9 @@ static void dev_change_rx_flags(struct net_device
*dev, int flags)
{
const struct net_device_ops *ops = dev->netdev_ops;
- if ((dev->flags & IFF_UP) && ops->ndo_change_rx_flags)
+ if (((dev->flags & IFF_UP) ||
+ (dev->flags & (IFF_MASTER | IFF_SLAVE)))
+ && ops->ndo_change_rx_flags)
ops->ndo_change_rx_flags(dev, flags);
}
--
1.8.4.2
why the differences? The one you send first works fine - have not tried v3.
Greets,
Stefan
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCHv3] net: core: Always propagate flag changes to interfaces
2013-11-20 7:21 ` Stefan Priebe - Profihost AG
@ 2013-11-20 13:54 ` Vlad Yasevich
2013-11-20 15:05 ` Stefan Priebe - Profihost AG
0 siblings, 1 reply; 5+ messages in thread
From: Vlad Yasevich @ 2013-11-20 13:54 UTC (permalink / raw)
To: Stefan Priebe - Profihost AG, netdev; +Cc: vfalico
On 11/20/2013 02:21 AM, Stefan Priebe - Profihost AG wrote:
> Hi,
>
> Am 20.11.2013 02:47, schrieb Vlad Yasevich:
>> The following commit:
>> b6c40d68ff6498b7f63ddf97cf0aa818d748dee7
>> net: only invoke dev->change_rx_flags when device is UP
>>
>> tried to fix a problem with VLAN devices and promiscuouse flag setting.
>> The issue was that VLAN device was setting a flag on an interface that
>> was down, thus resulting in bad promiscuity count.
>> This commit blocked flag propagation to any device that is currently
>> down.
>>
>> A later commit:
>> deede2fabe24e00bd7e246eb81cd5767dc6fcfc7
>> vlan: Don't propagate flag changes on down interfaces
>>
>> fixed VLAN code to only propagate flags when the VLAN interface is up,
>> thus fixing the same issue as above, only localized to VLAN.
>>
>> The problem we have now is that if we have create a complex stack
>> involving multiple software devices like bridges, bonds, and vlans,
>> then it is possible that the flags would not propagate properly to
>> the physical devices. A simple examle of the scenario is the
>> following:
>>
>> eth0----> bond0 ----> bridge0 ---> vlan50
>>
>> If bond0 or eth0 happen to be down at the time bond0 is added to
>> the bridge, then eth0 will never have promisc mode set which is
>> currently required for operation as part of the bridge. As a
>> result, packets with vlan50 will be dropped by the interface.
>>
>> The only 2 devices that implement the special flag handling are
>> VLAN and DSA and they both have required code to prevent incorrect
>> flag propagation. As a result we can remove the generic solution
>> introduced in b6c40d68ff6498b7f63ddf97cf0aa818d748dee7 and leave
>> it to the individual devices to decide whether they will block
>> flag propagation or not.
>>
>> Reported-by: Stefan Priebe <s.priebe@profihost.ag>
>> Suggested-by: Veaceslav Falico <vfalico@redhat.com>
>> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
>> ---
>> v2->v3: Removed a strange chunk that modified comments. Not sure where it
>> came from.
>>
>> net/core/dev.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index 974143d..da9c5e1 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -4991,7 +4991,7 @@ static void dev_change_rx_flags(struct net_device *dev, int flags)
>> {
>> const struct net_device_ops *ops = dev->netdev_ops;
>>
>> - if ((dev->flags & IFF_UP) && ops->ndo_change_rx_flags)
>> + if (ops->ndo_change_rx_flags)
>> ops->ndo_change_rx_flags(dev, flags);
>> }
>
> thanks for this patch - in one of the first posts you send this one:
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index fc913f4..016857b 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -4525,7 +4525,9 @@ static void dev_change_rx_flags(struct net_device
> *dev, int flags)
> {
> const struct net_device_ops *ops = dev->netdev_ops;
>
> - if ((dev->flags & IFF_UP) && ops->ndo_change_rx_flags)
> + if (((dev->flags & IFF_UP) ||
> + (dev->flags & (IFF_MASTER | IFF_SLAVE)))
> + && ops->ndo_change_rx_flags)
> ops->ndo_change_rx_flags(dev, flags);
> }
>
The new one should work just as well. The first one I had you try
removed the restriction for master and slave devices such as bonds
and their slaves. As Veaceslav pointed out, we can simply remove all
restrictions in this code and have the individual drivers do the right
thing. The 2 effected drivers are VLAN and DSA and they already do the
right checks before propagating flags.
I ran the same test with the simplified code and it worked for me
correctly setting promisc mode in the same set-up you had:
(phys dev) ---> bond ----> bridge ---> vlan
-vlad
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCHv3] net: core: Always propagate flag changes to interfaces
2013-11-20 13:54 ` Vlad Yasevich
@ 2013-11-20 15:05 ` Stefan Priebe - Profihost AG
0 siblings, 0 replies; 5+ messages in thread
From: Stefan Priebe - Profihost AG @ 2013-11-20 15:05 UTC (permalink / raw)
To: vyasevic, netdev; +Cc: vfalico
thanks, works fine.
Am 20.11.2013 14:54, schrieb Vlad Yasevich:
> On 11/20/2013 02:21 AM, Stefan Priebe - Profihost AG wrote:
>> Hi,
>>
>> Am 20.11.2013 02:47, schrieb Vlad Yasevich:
>>> The following commit:
>>> b6c40d68ff6498b7f63ddf97cf0aa818d748dee7
>>> net: only invoke dev->change_rx_flags when device is UP
>>>
>>> tried to fix a problem with VLAN devices and promiscuouse flag setting.
>>> The issue was that VLAN device was setting a flag on an interface that
>>> was down, thus resulting in bad promiscuity count.
>>> This commit blocked flag propagation to any device that is currently
>>> down.
>>>
>>> A later commit:
>>> deede2fabe24e00bd7e246eb81cd5767dc6fcfc7
>>> vlan: Don't propagate flag changes on down interfaces
>>>
>>> fixed VLAN code to only propagate flags when the VLAN interface is up,
>>> thus fixing the same issue as above, only localized to VLAN.
>>>
>>> The problem we have now is that if we have create a complex stack
>>> involving multiple software devices like bridges, bonds, and vlans,
>>> then it is possible that the flags would not propagate properly to
>>> the physical devices. A simple examle of the scenario is the
>>> following:
>>>
>>> eth0----> bond0 ----> bridge0 ---> vlan50
>>>
>>> If bond0 or eth0 happen to be down at the time bond0 is added to
>>> the bridge, then eth0 will never have promisc mode set which is
>>> currently required for operation as part of the bridge. As a
>>> result, packets with vlan50 will be dropped by the interface.
>>>
>>> The only 2 devices that implement the special flag handling are
>>> VLAN and DSA and they both have required code to prevent incorrect
>>> flag propagation. As a result we can remove the generic solution
>>> introduced in b6c40d68ff6498b7f63ddf97cf0aa818d748dee7 and leave
>>> it to the individual devices to decide whether they will block
>>> flag propagation or not.
>>>
>>> Reported-by: Stefan Priebe <s.priebe@profihost.ag>
>>> Suggested-by: Veaceslav Falico <vfalico@redhat.com>
>>> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
>>> ---
>>> v2->v3: Removed a strange chunk that modified comments. Not sure where it
>>> came from.
>>>
>>> net/core/dev.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/net/core/dev.c b/net/core/dev.c
>>> index 974143d..da9c5e1 100644
>>> --- a/net/core/dev.c
>>> +++ b/net/core/dev.c
>>> @@ -4991,7 +4991,7 @@ static void dev_change_rx_flags(struct net_device *dev, int flags)
>>> {
>>> const struct net_device_ops *ops = dev->netdev_ops;
>>>
>>> - if ((dev->flags & IFF_UP) && ops->ndo_change_rx_flags)
>>> + if (ops->ndo_change_rx_flags)
>>> ops->ndo_change_rx_flags(dev, flags);
>>> }
>>
>> thanks for this patch - in one of the first posts you send this one:
>>
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index fc913f4..016857b 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -4525,7 +4525,9 @@ static void dev_change_rx_flags(struct net_device
>> *dev, int flags)
>> {
>> const struct net_device_ops *ops = dev->netdev_ops;
>>
>> - if ((dev->flags & IFF_UP) && ops->ndo_change_rx_flags)
>> + if (((dev->flags & IFF_UP) ||
>> + (dev->flags & (IFF_MASTER | IFF_SLAVE)))
>> + && ops->ndo_change_rx_flags)
>> ops->ndo_change_rx_flags(dev, flags);
>> }
>>
>
> The new one should work just as well. The first one I had you try
> removed the restriction for master and slave devices such as bonds
> and their slaves. As Veaceslav pointed out, we can simply remove all
> restrictions in this code and have the individual drivers do the right
> thing. The 2 effected drivers are VLAN and DSA and they already do the
> right checks before propagating flags.
>
> I ran the same test with the simplified code and it worked for me
> correctly setting promisc mode in the same set-up you had:
>
> (phys dev) ---> bond ----> bridge ---> vlan
>
> -vlad
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCHv3] net: core: Always propagate flag changes to interfaces
2013-11-20 1:47 [PATCHv3] net: core: Always propagate flag changes to interfaces Vlad Yasevich
2013-11-20 7:21 ` Stefan Priebe - Profihost AG
@ 2013-11-20 20:30 ` David Miller
1 sibling, 0 replies; 5+ messages in thread
From: David Miller @ 2013-11-20 20:30 UTC (permalink / raw)
To: vyasevic; +Cc: netdev, s.priebe, vfalico
From: Vlad Yasevich <vyasevic@redhat.com>
Date: Tue, 19 Nov 2013 20:47:15 -0500
> The following commit:
> b6c40d68ff6498b7f63ddf97cf0aa818d748dee7
> net: only invoke dev->change_rx_flags when device is UP
>
> tried to fix a problem with VLAN devices and promiscuouse flag setting.
> The issue was that VLAN device was setting a flag on an interface that
> was down, thus resulting in bad promiscuity count.
> This commit blocked flag propagation to any device that is currently
> down.
>
> A later commit:
> deede2fabe24e00bd7e246eb81cd5767dc6fcfc7
> vlan: Don't propagate flag changes on down interfaces
>
> fixed VLAN code to only propagate flags when the VLAN interface is up,
> thus fixing the same issue as above, only localized to VLAN.
>
> The problem we have now is that if we have create a complex stack
> involving multiple software devices like bridges, bonds, and vlans,
> then it is possible that the flags would not propagate properly to
> the physical devices. A simple examle of the scenario is the
> following:
>
> eth0----> bond0 ----> bridge0 ---> vlan50
>
> If bond0 or eth0 happen to be down at the time bond0 is added to
> the bridge, then eth0 will never have promisc mode set which is
> currently required for operation as part of the bridge. As a
> result, packets with vlan50 will be dropped by the interface.
>
> The only 2 devices that implement the special flag handling are
> VLAN and DSA and they both have required code to prevent incorrect
> flag propagation. As a result we can remove the generic solution
> introduced in b6c40d68ff6498b7f63ddf97cf0aa818d748dee7 and leave
> it to the individual devices to decide whether they will block
> flag propagation or not.
>
> Reported-by: Stefan Priebe <s.priebe@profihost.ag>
> Suggested-by: Veaceslav Falico <vfalico@redhat.com>
> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
Applied and queued up for -stable, thanks Vlad.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-11-20 20:30 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-11-20 1:47 [PATCHv3] net: core: Always propagate flag changes to interfaces Vlad Yasevich
2013-11-20 7:21 ` Stefan Priebe - Profihost AG
2013-11-20 13:54 ` Vlad Yasevich
2013-11-20 15:05 ` Stefan Priebe - Profihost AG
2013-11-20 20:30 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).