hercules-390 / hyperion

Hercules 390
Other
246 stars 69 forks source link

QETH not forwarding broadcast frames in Layer 2 mode #211

Closed ivan-w closed 7 years ago

ivan-w commented 7 years ago

Issue exists on various OSes but confirmed on linux :

When the QETH interface is used in Layer 2 mode and is attached to a linux guest, the ARP requests sent from a remote host on the lan with a broadcast layer 2 mac address (ff:ff:ff:ff:ff:ff) is not being forwarded to the guest.

When the ethernet interface is set to promiscuous mode, the broadcast is received by the linux guest.

Unicast packets are received without an issue.

ivan-w commented 7 years ago

More on this.. Added the following at the beginning of validate_mac() in qeth.c and the problem no longer appears.

(Note it is ONLY a test, not a solution)

/ TEST / for(i=0;i<IFHWADDRLEN;i++) { if(mac[i]!=0xff) break; } if(i==(IFHWADDRLEN)) return MAC_TYPE_BRDCST;


This shouldn't be necessary as ff:ff:ff:ff:ff:ff is registered for each QETH device group at initialization as a broadcast MAC. So something else must be happening somewhere else.

Tested with linux and z/VM 6.3

mcisho commented 7 years ago

Does qeth debug give any clue?

ivan-w commented 7 years ago

Not really.

But it's obvious that if I force broadcast frames to be accepted in validate_mac(), then everything works fine.

Things also work fine if the QETH instance is set to promiscuous mode.

ivan-w commented 7 years ago

But I'm thinking of adding a "qeth macs" to show the registered MACS for a specific QETH Instance

ivan-w commented 7 years ago

My feeling is that the register of the broadcast MAC is being invalidated somehow.

mcisho commented 7 years ago

register_mac says:- if(!grp->mac[i].type || !memcmp(grp->mac[i].addr,mac,IFHWADDRLEN)) I'm feeling tired and sleepy so I may be confused, but doesn't that always overwrite the first entry?

Fish-Git commented 7 years ago

Added the following at the beginning of validate_mac() in qeth.c and the problem no longer appears.

(Note it is ONLY a test, not a solution)

/ TEST / for(i=0;i<IFHWADDRLEN;i++) { if(mac[i]!=0xff) break; } if(i==(IFHWADDRLEN)) return MAC_TYPE_BRDCST;

FYI: The above could be reduced to just the following:

if (mac[0] & 0x01)
    return MAC_TYPE_BRDCST;

If the 0x01 bit is on in the first byte of the MAC then it's by definition an all stations broadcast frame.

ivan-w commented 7 years ago

Fish,

If the last bit of of the first byte of the MAC address is 1, it only indicates to switches to send the frame to all ports (except the port on which the frame was originally received). It is a MULTICAST frame. Individual network interfaces may elect not to receive/ignore certain multicast frames except for the special multicast address consisting of all binary 1s - which is the broadcast MAC address - which should always be accepted.

https://en.wikipedia.org/wiki/MAC_address#Unicast_vs._multicast

ivan-w commented 7 years ago

committed 42841ce19756de72fdfdcd1d98b49f569339f88d

ivan-w commented 7 years ago

Tested here and fixes the issue. Will close in 24h unless the issue is causing other problems.

mcisho commented 7 years ago

Now I'm awake and have implemented Ivan's idea of adding a "qeth addr" to show the registered MAC and IP addresses for all QETH instances, I can see that register_mac saying:- if(!grp->mac[i].type || !memcmp(grp->mac[i].addr,mac,IFHWADDRLEN)) doesn't always overwrite the first entry, but it does overwrite the first entry. Once I have tidied up the command, and have fixed register_mac (which will include removing Ivan's fix), I'll do a commit.

ivan-w commented 7 years ago

I'm only worried about remove_all_macs being invoked ;) (Edited : I meant : unregister_all_mac())

It would remove accepting broadcast frames registered during initial instantiation. Accepting broadcast frames should NEVER be removed.

Accepting broadcast frames shouldn't need registering and should be accepted no matter what. (it is a burnt in capability).

So we either : Accept broadcast frames in validate_mac() or we unregister all MAC addresses except the broadcast frame in unregister_all_mac().

--Ivan

mcisho commented 7 years ago

I agree. I had decided it would be better to keep your fix, i.e. always accept broadcast in validate_mac, and not register the broadcast address, so that QETH only registers those addresses supplied by the guest.

ivan-w commented 7 years ago

I think we concur there !

However, I'd like the qeth macs - or qeth addrs... capability - so one can examine what MAC addresses (or IPv4/IPv6 for a Layer 3 QETH) are being accepted on a QETH interface - if you have already work done on this, I'd love to see it implemented !

Thanks a lot for the feedback and taking a thorough look at this ! Greatly appreciated !

Fish-Git commented 7 years ago

If the last bit of of the first byte of the MAC address is 1, it only indicates to switches to send the frame to all ports (except the port on which the frame was originally received). It is a MULTICAST frame. Individual network interfaces may elect not to receive/ignore certain multicast frames except for the special multicast address consisting of all binary 1s - which is the broadcast MAC address - which should always be accepted.

(Oops!) You are of course correct, Ian. I was only looking at the code fragment from a "Should it be accepted or ignored?" incoming frame point of view. For qeth.c's validate_mac function however, distinguishing between the two is probably important. :)

Good catch. Thank you for keeping me on my toes.

Fish-Git commented 7 years ago

I'd like the qeth macs - or qeth addrs... capability - so one can examine what MAC addresses (or IPv4/IPv6 for a Layer 3 QETH) are being accepted on a QETH interface - if you have already work done on this, I'd love to see it implemented !

I agree! I can see how having such a function (command) could prove to be quite handy during troubleshooting, so I too would like to see it!

ivan-w commented 7 years ago

Ian has added a bit of more verbosity to show which address are being registered.

I think I'll close the issue. Everything seems to be working as expected on my side now.