login
Search
IFF_PROMISC should be set before IFF_ALLMULTI.
Signed-off-by: Wang Chen <wangchen@cn.fujitsu.com>
---
net/8021q/vlan_dev.c | 25 +++++++++++++++++++------
1 files changed, 19 insertions(+), 6 deletions(-)
diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index 5d055c2..14742e3 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -547,10 +547,14 @@ static int vlan_dev_open(struct net_device *dev)
}
memcpy(vlan->real_dev_addr, real_dev->dev_addr, ETH_ALEN);
- if (dev->flags & IFF_ALLMULTI)
- dev_set_allmulti(real_dev, 1);
+ /* NOTE: order of synchronization of IFF_PROMISC and IFF_ALLMULTI
+ is important. Some (broken) drivers set IFF_PROMISC, when
+ IFF_ALLMULTI is requested not asking us and not reporting.
+ */
if (dev->flags & IFF_PROMISC)
dev_set_promiscuity(real_dev, 1);
+ if (dev->flags & IFF_ALLMULTI)
+ dev_set_allmulti(real_dev, 1);
return 0;
}
@@ -561,10 +565,14 @@ static int vlan_dev_stop(struct net_device *dev)
dev_mc_unsync(real_dev, dev);
dev_unicast_unsync(real_dev, dev);
- if (dev->flags & IFF_ALLMULTI)
- dev_set_allmulti(real_dev, -1);
+ /* NOTE: order of synchronization of IFF_PROMISC and IFF_ALLMULTI
+ is important. Some (broken) drivers set IFF_PROMISC, when
+ IFF_ALLMULTI is requested not asking us and not reporting.
+ */
if (dev->flags & IFF_PROMISC)
dev_set_promiscuity(real_dev, -1);
+ if (dev->flags & IFF_ALLMULTI)
+ dev_set_allmulti(real_dev, -1);
if (compare_ether_addr(dev->dev_addr, real_dev->dev_addr))
dev_unicast_delete(real_dev, dev->dev_addr, dev->addr_len);
@@ -626,10 +634,15 @@ static void vlan_dev_change_rx_flags(struct net_device *dev, int change)
{
struct net_device *real_dev = vlan_dev_info(dev)->real_dev;
+ /* NOTE: order of synchronization of IFF_PROMISC and IFF_ALLMULTI
+ is important. Some (broken) drivers set IFF_PROMISC, when
+ IFF_ALLMULTI is requested not asking us and not reporting.
+ */
+ if (change & ...What exactly is the problem here? The VLAN code is obviously not one of the broken drivers, so why should it care what other drivers do? --
I think the problem is that allmulti is not valid if promis is not on. And about the comment, I copy it from dev_change_flags() and think it seems suit for here. Did I misunderstand this comment? --
I think it refers to broken behaviour by drivers that set IFF_PROMISC themselves when asked to disable multicast filtering by setting IFF_ALLMULTI. This would cause the test for changed flags in dev_set_promiscuity to return zero and not program the device for promiscous mode properly. There are a few examples of this in the tree. But calling dev_set_promiscuity() before dev_set_allmulti() only helps in the dev_change_flags() case since its the only function that might change both flags at once. In all other cases it depends on the caller. So for the dev_change_flags() case VLAN already uses the "proper" ordering, the other cases might be broken with or without your patch. I'd suggest to fix the drivers instead, perhaps start by adding a warning to dev_change_flags() that is triggered by the driver changing the flags itself. --
In some driver's code of *_set_multicast_list(), IFF_PROMISC will be set if IFF_ALLMULTI is set. And there is comment about the necessity for setting IFF_PROMISC. /* * We must make the kernel realise we had to move * into promisc mode or we start all out war on * the cable. If it was a promisc request the * flag is already set. If not we assert it. */ So, I doubt about fixing the drivers. --
If that ordering is really required, yes: - ip link set dev eth0 allmulticast on <sets allmulticast *and* promisous with broken driver> - ip link set dev eth0 promisc on <no change> So the only thing fixed by this workaround is of both are enabled in a single command - something that doesn't even make much sense since promisc will receive all multicast I have no idea what this comment is trying to say. Drivers shouldn't change dev->flags. --
Yes. Maybe we should ask the authors about why they let driver to change the dev->flags. Fox example, about 3c527. Alan, Can you explain why do_mc32_set_multicast_list() change flag to IFF_PROMISC when IFF_ALLMULTI is set? Jeff, Any suggestion about this? netdevice uses promiscuity as refcnt, and if driver set IFF_PROMISC but don't change promiscuity will break the refcnt. --
Drivers should not be setting IFF_* flags in set_multicast_list(). The normal logic is that a driver interprets the request implied in set_multicast_list ("promisc, all-multi, or select multi?"), and then programs the hardware based on that. On some hardware, IFF_ALLMULTI requires that the hardware receive all packets (promisc). Even for that case, the driver should -not- be setting the IFF_PROMISC flag. It should be aware of its own hardware programming state through some other method. Jeff --
OK. Roger that. If Alan does not oppose it, I will send patches to these drivers later. Thank you :-) --
Did you check that these drivers don't use the PROMISC flag they set themselves somewhere? As Jeff said, they might use it to be Shouldn't this be using dev_set_promiscuity(). --
Yes. I checked. The flag is set but not be used anywhere else. All of the drivers set their own state and at the same time set IFF_PROMIDC flag. I think that by setting IFF_PROMISC the drivers want to inform upper layer that they set hardware to promisc although they are requested to set ALLMULTI. But the driver's redundant action is unneeded. Because, if the hardwares have to set promisc mode when they required to receive all multicast packets, it's ok, upper layer don't need to be informed. No. 1. dev_set_promiscuity do a. set/unset IFF_PROMISC b. promiscuity++/-- c. audit d. dev_set_rx_mode (upload unicast and multicast list to device) Here, in ioctl, a & b is enough. 2. dev->flags unset IFF_PROMISC and dev->promiscuity = 0 can not be replaced by dev_set_promiscuity(). Because, we don't decrease promiscuity here, but set promiscuity zero for unset IFF_PROMISC. --
Auditing should certainly be done if promiscous mode is set. Calling dev_set_rx_mode doesn't hurt, even if it does the ioctl handler could be changed not to care. Besides this is neither taking the rtnl_mutex as required nor sending notifcations And that looks like a bug, the driver shouldn't disable promiscuity if something still requires it. --
dev_change_flags() can not completely change flag IFF_PROMISC like IFF_UP, etc. So dev_change_flags() has no big difference to dev_set_promiscuity(). It's hard to say that. In theory, user-space can require device to disable promisc by driver's ioctl. But OTOH if something else still want device to be promisc, user and driver have no method to let them decrease the refcnt promiscuity. Because promiscuity decrement is initiative action from upper layer, drivers don't know who need promiscuity. Humm, tired, go to sleep and figure out how to do after refreshing. :) --
It doesn't send notifications and this should always be It can't normally, only this driver can. And that doesn't I'd suggest to make it user dev_change_flags() and maybe even print a warning and add these ioctls to feature-removal-schedule. --
Ooh, there is a call_netdevice_notifiers() in dev_change_flags(). Yes. I also agree. Except that if use dev_change_flags() for "case DE4X5_CLR_PROM", we will break the logic of this code. Because it sets hardware to non-promisc, but dev_change_flags() can only decrease the refcnt and do not clear IFF_PROMISC. The hw side and software side will be inconsistent. So I want to remove "clear promisc" feature by ioctl of this driver instead of adding it to feature-removal-schedule. "set promisc" feature by ioctl can be fixed temporarily and should be added to feature-removal-schedule. Fortunately most of the features of this driver's ioctl are for developer testing. This week I want to wait for tulip driver maintainer's confirmation. --
On Tue, Jun 24, 2008 at 09:02:03AM +0800, Wang Chen wrote: Confirmation of what? Apologies for not being able to follow the whole conversation. If you want to remove a developer debug feature, please just submit a patch like usual to kyle and myself. thanks, grant --
Sorry for that I cc-ed you in the mid of discussion. Patrick and me were discussing about that driver should not change device's promisc mode, because it's not a boolean state, but a refcnt to decide whether IFF_PROMISC should be set or not. In de4x5_ioctl(), user can simply set IFF_PROMISC flag and that can cause upper layer protocol broken. Patrick suggest to use dev_change_flags() to change IFF_PROMISC flag and add the feature to feature-removal-schedule. My suggestion is that we remove IFF_PROMISC changing feature from Is there any user space binary use these features? case DE4X5_SET_PROM: case DE4X5_CLR_PROM: If not, I will send a patch to remove them. --
IFF_PROMISC flag shouldn't be set or cleared by drivers, because
whether device be promisc mode is decided by how many upper layer
callers being referenced to it.
And the promisc changing feature of de4x5 ioctl is developer debug
feature, we can remove it now.
Signed-off-by: Wang Chen <wangchen@cn.fujitsu.com>
---
diff --git a/drivers/net/tulip/de4x5.c b/drivers/net/tulip/de4x5.c
index bc30c6e..617ef41 100644
--- a/drivers/net/tulip/de4x5.c
+++ b/drivers/net/tulip/de4x5.c
@@ -5514,22 +5514,6 @@ de4x5_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
netif_wake_queue(dev); /* Unlock the TX ring */
break;
- case DE4X5_SET_PROM: /* Set Promiscuous Mode */
- if (!capable(CAP_NET_ADMIN)) return -EPERM;
- omr = inl(DE4X5_OMR);
- omr |= OMR_PR;
- outl(omr, DE4X5_OMR);
- dev->flags |= IFF_PROMISC;
- break;
-
- case DE4X5_CLR_PROM: /* Clear Promiscuous Mode */
- if (!capable(CAP_NET_ADMIN)) return -EPERM;
- omr = inl(DE4X5_OMR);
- omr &= ~OMR_PR;
- outl(omr, DE4X5_OMR);
- dev->flags &= ~IFF_PROMISC;
- break;
-
case DE4X5_SAY_BOO: /* Say "Boo!" to the kernel log file */
if (!capable(CAP_NET_ADMIN)) return -EPERM;
printk("%s: Boo!\n", dev->name);
diff --git a/drivers/net/tulip/de4x5.h b/drivers/net/tulip/de4x5.h
index f5f33b3..a6b7638 100644
--- a/drivers/net/tulip/de4x5.h
+++ b/drivers/net/tulip/de4x5.h
@@ -1004,8 +1004,8 @@ struct de4x5_ioctl {
*/
#define DE4X5_GET_HWADDR 0x01 /* Get the hardware address */
#define DE4X5_SET_HWADDR 0x02 /* Set the hardware address */
-#define DE4X5_SET_PROM 0x03 /* Set Promiscuous Mode */
-#define DE4X5_CLR_PROM 0x04 /* Clear Promiscuous Mode */
+#define DE4X5_SET_PROM 0x03 /* Obsoleted. Set Promiscuous Mode */
+#define DE4X5_CLR_PROM 0x04 /* Obsoleted. Clear Promiscuous Mode */
#define DE4X5_SAY_BOO 0x05 /* Say "Boo!" to the kernel log file */
#define DE4X5_GET_MCA 0x06 /* Get a multicast address */
#define DE4X5_SET_MCA ...I spent about 20 minutes using both google and yahoo search trying to find a use and did not. Given this driver has been visible Please just remove them. If the kernel isn't going to provide the functionality, I'd rather any apps that attempt to use it break thanks, grant --
OK. I removed the macros, but left comment to prevent people from reusing
the code 0x03 and 0x04.
---
IFF_PROMISC flag shouldn't be set or cleared by drivers, because
whether device be promisc mode is decided by how many upper layer
callers being referenced to it.
And the promisc changing feature of de4x5 ioctl is developer debug
feature, we can remove it now.
Signed-off-by: Wang Chen <wangchen@cn.fujitsu.com>
---
diff --git a/drivers/net/tulip/de4x5.c b/drivers/net/tulip/de4x5.c
index bc30c6e..617ef41 100644
--- a/drivers/net/tulip/de4x5.c
+++ b/drivers/net/tulip/de4x5.c
@@ -5514,22 +5514,6 @@ de4x5_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
netif_wake_queue(dev); /* Unlock the TX ring */
break;
- case DE4X5_SET_PROM: /* Set Promiscuous Mode */
- if (!capable(CAP_NET_ADMIN)) return -EPERM;
- omr = inl(DE4X5_OMR);
- omr |= OMR_PR;
- outl(omr, DE4X5_OMR);
- dev->flags |= IFF_PROMISC;
- break;
-
- case DE4X5_CLR_PROM: /* Clear Promiscuous Mode */
- if (!capable(CAP_NET_ADMIN)) return -EPERM;
- omr = inl(DE4X5_OMR);
- omr &= ~OMR_PR;
- outl(omr, DE4X5_OMR);
- dev->flags &= ~IFF_PROMISC;
- break;
-
case DE4X5_SAY_BOO: /* Say "Boo!" to the kernel log file */
if (!capable(CAP_NET_ADMIN)) return -EPERM;
printk("%s: Boo!\n", dev->name);
diff --git a/drivers/net/tulip/de4x5.h b/drivers/net/tulip/de4x5.h
index f5f33b3..9f28774 100644
--- a/drivers/net/tulip/de4x5.h
+++ b/drivers/net/tulip/de4x5.h
@@ -1004,8 +1004,7 @@ struct de4x5_ioctl {
*/
#define DE4X5_GET_HWADDR 0x01 /* Get the hardware address */
#define DE4X5_SET_HWADDR 0x02 /* Set the hardware address */
-#define DE4X5_SET_PROM 0x03 /* Set Promiscuous Mode */
-#define DE4X5_CLR_PROM 0x04 /* Clear Promiscuous Mode */
+/* 0x03 and 0x04 were used before and are obsoleted now. Don't use them. */
#define DE4X5_SAY_BOO 0x05 /* Say "Boo!" to the kernel log file */
#define DE4X5_GET_MCA 0x06 /* Get a ...Diff from v1 to v2: split de4x5's patch from previews one, because
it is a little bit different to others.
---
Some hardware set promisc when they are requested to set IFF_ALLMULTI flag.
It's ok, but if drivers set IFF_PROMISC flag when they set promisc,
it will broken upper layer handle for promisc and allmulti.
In addition, drivers can use their own hardware programming to make it.
So do not allow drivers to set IFF_* flags.
This is a general driver fix, so I didn't split it to pieces and send
to specific driver maintainers.
Signed-off-by: Wang Chen <wangchen@cn.fujitsu.com>
---
diff --git a/drivers/net/3c523.c b/drivers/net/3c523.c
index 239fc42..9e48660 100644
--- a/drivers/net/3c523.c
+++ b/drivers/net/3c523.c
@@ -641,10 +641,8 @@ static int init586(struct net_device *dev)
cfg_cmd->time_low = 0x00;
cfg_cmd->time_high = 0xf2;
cfg_cmd->promisc = 0;
- if (dev->flags & (IFF_ALLMULTI | IFF_PROMISC)) {
+ if (dev->flags & (IFF_ALLMULTI | IFF_PROMISC))
cfg_cmd->promisc = 1;
- dev->flags |= IFF_PROMISC;
- }
cfg_cmd->carr_coll = 0x00;
p->scb->cbl_offset = make16(cfg_cmd);
diff --git a/drivers/net/3c527.c b/drivers/net/3c527.c
index fae295b..a0bea51 100644
--- a/drivers/net/3c527.c
+++ b/drivers/net/3c527.c
@@ -1524,14 +1524,11 @@ static void do_mc32_set_multicast_list(struct net_device *dev, int retry)
struct mc32_local *lp = netdev_priv(dev);
u16 filt = (1<<2); /* Save Bad Packets, for stats purposes */
- if (dev->flags&IFF_PROMISC)
+ if ((dev->flags&IFF_PROMISC) ||
+ (dev->flags&IFF_ALLMULTI) ||
+ dev->mc_count > 10)
/* Enable promiscuous mode */
filt |= 1;
- else if((dev->flags&IFF_ALLMULTI) || dev->mc_count > 10)
- {
- dev->flags|=IFF_PROMISC;
- filt |= 1;
- }
else if(dev->mc_count)
{
unsigned char block[62];
diff --git a/drivers/net/atp.c b/drivers/net/atp.c
index 3d44333..c10cd80 100644
--- a/drivers/net/atp.c
+++ b/drivers/net/atp.c
@@ -854,14 +854,9 @@ static void set_rx_mode_8002(struct net_device ...
