Bug 481283

 4. State specific action requested of SEG
   I have looked in the git. Below is upstream patch requested by the vendor:
     8c979c26a0f093c13290320edda799d8335e50ae
   Apparently this may need some more patches as:
     (vlan_group_get_device)
     5c15bdec5c38f4ccf73ef2585fc80a6164de9554
     (dev_unicast_delete/dev_unicast_add)
     4417da668c0021903464f92db278ddae348e0299
   
Issue escalated to Support Engineering Group by: tumeya.

Created attachment 329816 [details]
5c15bdec5c38f4ccf73ef2585fc80a6164de9554.patch

Created attachment 329817 [details]
8c979c26a0f093c13290320edda799d8335e50ae.patch

Created attachment 329818 [details]
4417da668c0021903464f92db278ddae348e0299.patch

Created attachment 329819 [details]
patch by customer based on kernel-2.6.18-92.1.6.el5.ia64

Updating PM score.

I'll try this myself on monday, but just in case you already have test results, is the problem reproducible upstream (or on an F10/F11 system)?

I'm going to try this on an upstream kernel to see if there is already a fix in existance that we can use.  I should note however, that the state change on the promisc bit is a bit suspect, but the multicast bit should not have any relevance to this problem.  All the multicast bit indicates is that the interface can send and recieve multicast frames, and for hardware that supports it, that flag should be set automatically, and only cleared administratively .

Created attachment 344499 [details]
patch to track and undo promisc count on vlans

Ok, looks like upstream fixed this via  a pretty big rewrite on how we handle unicast lists.  Can you try this patch, it seems somewhat more consice to me.  I've not tested it yet, but it should work just fine.

Also, I noticed in the reprouder given, the customer tried to undo the promisc flag by setting -rpromisc.  Not sure if that was just a typo, but it should be -promisc instead.  Either way, if lots of vlans are being used, it would still be broken, as the promisc count will be off (you would have to issue that command several times to clear the flag)

Created attachment 344731 [details]
vlan-with-fixes-1.patch

I did try fedora 10 running 2.6.10-rc6 and it worked fine. Actually, because of vlan_dev_set_mac_address() uses dev_unicast_*() it doesn't set promisc anymore.

I also tried -128.el5 and could see the problem happening then I applied your patch with two extra changes. One is a simple typo and another is moving  'VLAN_DEV_INFO(dev)->promisc_count * -1;' to inside of if clause, otherwise dev is NULL and it would oops.
...
 	struct net_device *dev = NULL;
 	int ret;
-
+	int undo_count = VLAN_DEV_INFO(dev)->promisc_count * -1;
 
 	dev = dev_get_by_name(vlan_IF_name);
...
modified patch is attached in case you need to check something.

Anyway, it didn't work because on -128.el5 does:
vlan_dev_set_mac_address()
  /* Increment our in-use promiscuity counter */
  dev_set_promiscuity(VLAN_DEV_INFO(dev)->real_dev, 1); <-- increments 1
  VLAN_DEV_INFO(dev)->promisc_count++;                  <-- patch counts that
  flgs |= IFF_PROMISC;
  dev_change_flags(VLAN_DEV_INFO(dev)->real_dev, flgs);
  which does:
        if ((flags ^ dev->gflags) & IFF_PROMISC) {
                int inc = (flags & IFF_PROMISC) ? +1 : -1;
                dev->gflags ^= IFF_PROMISC;
                dev_set_promiscuity(dev, inc);   <--- increments another one.
        }

so, the counter ends with 2. Removing vlan device leaves the counter with 1.
Flavio

Ok, so you identified a spot I missed, thats great.  Did you test with the obvious fix for the missed spot, and did it work? :)

It will work regarding with dev->flags but I'm not finding how it will work with dev->gflags.

vlan_ioctl_handler()
   unregister_vlan_device()
      dev_set_promiscuity() <-- flags only.

Another thing, changing macaddr will set promisc, then if you change the macaddr back to the real_dev's macaddr, it will leave promisc flag set on both flags and gflags too. See vlan_dev_set_mac_address().

Flavio

yes, you found another problem.

You took the time to analyze it.  Thank you, again now, please take the extra 5 minutes to fix it.  You should just need to check the promiscuity value of the physical dev after chaning its promiscuity and change gflags if it returns to zero.

Created attachment 345357 [details]
vlan patch with gflags fixed

Another patch.

On vlan_dev_set_mac_address() if there is no IFF_PROMISC in flags, call dev_change_flags() and set gflags if needed. Otherwise assume flags and gflags are okay and just increment one.

On unregister_vlan_device() undo vlan promisc changes and if flags has left without IFF_PROMISC takes it out from gflags too.

works here.
Flavio

Thank you, yes.  This looks good to me.  I'll post it shortly.   Thank you for fixing up those missing bits.

This request was evaluated by Red Hat Product Management for inclusion in a Red
Hat Enterprise Linux maintenance release.  Product Management has requested
further review of this request by Red Hat Engineering, for potential
inclusion in a Red Hat Enterprise Linux Update release for currently deployed
products.  This request is not yet committed for inclusion in an Update
release.

in kernel-2.6.18-157.el5
You can download this test kernel from http://people.redhat.com/dzickus/el5

Please do NOT transition this bugzilla state to VERIFIED until our QE team
has sent specific instructions indicating when to do so.  However feel free
to provide a comment indicating that this fix has been verified.

An advisory has been issued which should help the problem
described in this bug report. This report is therefore being
closed with a resolution of ERRATA. For more information
on therefore solution and/or where to find the updated files,
please follow the link below. You may reopen this bug report
if the solution does not work for you.

http://rhn.redhat.com/errata/RHSA-2009-1243.html