getifaddrs does not fill in MAC address

justinc1 commented 8 years ago

Follow up to #783. nodeJS uses getifaddrs to figure out its MAC address, and this is not working. A minimal test program is attached - it 99% copy of uv_interface_addresses function. On Linux, MAC addresses are shown, while on OSv interfaces appear to "not be" "UP/RUNNING/PF_PACKET, or has NULL ifa_addr" and are skipped.

[xlab@mike-c7 osv]$ sudo ./scripts/run.py -d -e '/tst/tst-getifaddrs.so'
OSv v0.24-167-g144f05e
eth0: 192.168.122.15
TTRT linux-core.c:77 uv_interface_addresses
TTRT linux-core.c:91 uv_interface_addresses ERR getifaddrs OK
TTRT linux-core.c:98 uv_interface_addresses ifaddr=0xffffa000032c0d00 count=0
TTRT linux-core.c:98 uv_interface_addresses ifaddr=0xffffa000032c0600 count=1
TTRT linux-core.c:108 uv_interface_addresses almost-FINAL count=2
TTRT linux-core.c:113 uv_interface_addresses INFO *addresses=0xffffa00003241a00
/*---------------------------*/
INFO name lo0 is not UP/RUNNING/PF_PACKET, or has NULL ifa_addr
INFO name eth0 is not UP/RUNNING/PF_PACKET, or has NULL ifa_addr
TTRT linux-core.c:183 uv_interface_addresses THE-FINAL count=2

justinc1 commented 8 years ago

tst-getifaddrs.c.txt

benoit-canet commented 8 years ago

Thanks for the test case Justin.

I'll try to have a look tomorow at it.

On Tue, Aug 16, 2016 at 2:54 PM, justinc1 notifications@github.com wrote:

tst-getifaddrs.c.txt https://github.com/cloudius-systems/osv/files/420270/tst-getifaddrs.c.txt

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/cloudius-systems/osv/issues/787#issuecomment-240092620, or mute the thread https://github.com/notifications/unsubscribe-auth/AAA5IdUKW07ewV4A-ReoJV97tZ2doNmoks5qgbL-gaJpZM4JlWmn .

benoit-canet commented 8 years ago

I took some time to look at it and I confirm that the ether addr are all zeroes which is a bit strange.

I will work a bit more on it tomorow.

Best regards

Benoît

On Tue, Aug 16, 2016 at 3:36 PM, Benoît Canet benoit@cloudius-systems.com wrote:

Thanks for the test case Justin.

I'll try to have a look tomorow at it.

On Tue, Aug 16, 2016 at 2:54 PM, justinc1 notifications@github.com wrote:

tst-getifaddrs.c.txt https://github.com/cloudius-systems/osv/files/420270/tst-getifaddrs.c.txt

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/cloudius-systems/osv/issues/787#issuecomment-240092620, or mute the thread https://github.com/notifications/unsubscribe-auth/AAA5IdUKW07ewV4A-ReoJV97tZ2doNmoks5qgbL-gaJpZM4JlWmn .

benoit-canet commented 8 years ago

This is a musl (The libc OSv use) brokenness.

This commit: https://github.com/bpowers/musl/commit/08e4052c43692a9306c5c638d70fba7f7ba08c52 reimplement everything to provide mac addresses.

Alas netlink is probably not implemented in OSv so we cannot just backport this patch.

I think I will have to patch the current musl code.

Best regards

Benoît

On Tue, Aug 16, 2016 at 6:51 PM, Benoît Canet benoit@cloudius-systems.com wrote:

I took some time to look at it and I confirm that the ether addr are all zeroes which is a bit strange.

I will work a bit more on it tomorow.

Best regards

Benoît

On Tue, Aug 16, 2016 at 3:36 PM, Benoît Canet <benoit@cloudius-systems.com

wrote:

Thanks for the test case Justin.

I'll try to have a look tomorow at it.

On Tue, Aug 16, 2016 at 2:54 PM, justinc1 notifications@github.com wrote:

tst-getifaddrs.c.txt https://github.com/cloudius-systems/osv/files/420270/tst-getifaddrs.c.txt

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/cloudius-systems/osv/issues/787#issuecomment-240092620, or mute the thread https://github.com/notifications/unsubscribe-auth/AAA5IdUKW07ewV4A-ReoJV97tZ2doNmoks5qgbL-gaJpZM4JlWmn .

benoit-canet commented 8 years ago

Hi Justin,

I have a working patch.

Let me a bit of time to beat it into shape and I will post it to the osv list.

Best regards

Benoît

On Wed, Aug 17, 2016 at 5:31 PM, Benoît Canet benoit@cloudius-systems.com wrote:

This is a musl (The libc OSv use) brokenness.

This commit: https://github.com/bpowers/musl/commit/08e4052c43692a9306c5c638d70fba 7f7ba08c52 reimplement everything to provide mac addresses.

Alas netlink is probably not implemented in OSv so we cannot just backport this patch.

I think I will have to patch the current musl code.

Best regards

Benoît

On Tue, Aug 16, 2016 at 6:51 PM, Benoît Canet <benoit@cloudius-systems.com

wrote:

I took some time to look at it and I confirm that the ether addr are all zeroes which is a bit strange.

I will work a bit more on it tomorow.

Best regards

Benoît

On Tue, Aug 16, 2016 at 3:36 PM, Benoît Canet < benoit@cloudius-systems.com> wrote:

Thanks for the test case Justin.

I'll try to have a look tomorow at it.

On Tue, Aug 16, 2016 at 2:54 PM, justinc1 notifications@github.com wrote:

tst-getifaddrs.c.txt https://github.com/cloudius-systems/osv/files/420270/tst-getifaddrs.c.txt

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/cloudius-systems/osv/issues/787#issuecomment-240092620, or mute the thread https://github.com/notifications/unsubscribe-auth/AAA5IdUKW07ewV4A-ReoJV97tZ2doNmoks5qgbL-gaJpZM4JlWmn .

benoit-canet commented 8 years ago

Patch to test on the list.

On Wed, Aug 17, 2016 at 8:25 PM, Benoît Canet benoit@cloudius-systems.com wrote:

Hi Justin,

I have a working patch.

Let me a bit of time to beat it into shape and I will post it to the osv list.

Best regards

Benoît

On Wed, Aug 17, 2016 at 5:31 PM, Benoît Canet <benoit@cloudius-systems.com

wrote:

This is a musl (The libc OSv use) brokenness.

This commit: https://github.com/bpowers/musl/commit/08e4052c43692a9306c5c 638d70fba7f7ba08c52 reimplement everything to provide mac addresses.

Alas netlink is probably not implemented in OSv so we cannot just backport this patch.

I think I will have to patch the current musl code.

Best regards

Benoît

On Tue, Aug 16, 2016 at 6:51 PM, Benoît Canet < benoit@cloudius-systems.com> wrote:

I took some time to look at it and I confirm that the ether addr are all zeroes which is a bit strange.

I will work a bit more on it tomorow.

Best regards

Benoît

On Tue, Aug 16, 2016 at 3:36 PM, Benoît Canet < benoit@cloudius-systems.com> wrote:

Thanks for the test case Justin.

I'll try to have a look tomorow at it.

On Tue, Aug 16, 2016 at 2:54 PM, justinc1 notifications@github.com wrote:

tst-getifaddrs.c.txt https://github.com/cloudius-systems/osv/files/420270/tst-getifaddrs.c.txt

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/cloudius-systems/osv/issues/787#issuecomment-240092620, or mute the thread https://github.com/notifications/unsubscribe-auth/AAA5IdUKW07ewV4A-ReoJV97tZ2doNmoks5qgbL-gaJpZM4JlWmn .

justinc1 commented 8 years ago

The tst-getifadds.c still doesn't work. I see two problems.

With little modification:

  /* Fill in physical addresses for each interface */
  for (ent = addrs; ent != NULL; ent = ent->ifa_next) {
    if (!((ent->ifa_flags & IFF_UP) && (ent->ifa_flags & IFF_RUNNING)) ||
        (ent->ifa_addr == NULL) ||
        (ent->ifa_addr->sa_family != PF_PACKET)) {
      fprintf(stderr, "INFO name %s is not UP/RUNNING/PF_PACKET, or has NULL ifa_addr\n", ent->ifa_name);
      fprintf(stderr, "  ent->ifa_flags=0x%08x IFF_UP=0x%08x IFF_RUNNING=0x%08x\n", ent->ifa_flags, IFF_UP, IFF_RUNNING);
      fprintf(stderr, "  ent->ifa_addr=%p\n", ent->ifa_addr);
      fprintf(stderr, "  ent->ifa_addr->sa_family=0x%08x PF_PACKET=0x%08x\n", ent->ifa_addr==NULL? -1: ent->ifa_addr->sa_family, PF_PACKET);
      sleep(0);
      continue;
    }

The output is like:

/*---------------------------*/
INFO name lo0 is not UP/RUNNING/PF_PACKET, or has NULL ifa_addr
  ent->ifa_flags=0x00011049 IFF_UP=0x00000001 IFF_RUNNING=0x00000040
  ent->ifa_addr=0xffffa0000253bc40
  ent->ifa_addr->sa_family=0x00000000 PF_PACKET=0x00000011
INFO name eth0 is not UP/RUNNING/PF_PACKET, or has NULL ifa_addr
  ent->ifa_flags=0x00011043 IFF_UP=0x00000001 IFF_RUNNING=0x00000040
  ent->ifa_addr=0xffffa0000253bc50
  ent->ifa_addr->sa_family=0x00000000 PF_PACKET=0x00000011

So nodjs code fails due to ent->ifa_addr->sa_family=0.

2) Let me remove (ent->ifa_addr->sa_family != PF_PACKET) check, and output is:

sudo ./scripts/run.py -nvd -e '/tst/tst-getifaddrs.so; /cli/cli.so' 
...
/*---------------------------*/
TTRT lo0 mac=00:00:00:00:00:00
TTRT eth0 mac=52:00:00:00:70:bc

BUT the rest of the world sees eth0 mac 52:54:00:12:34:56.

nyh commented 8 years ago

On Thu, Aug 18, 2016 at 7:23 AM, justinc1 notifications@github.com wrote:

The tst-getifadds.c still doesn't work. I see two problems.

With little modification:

/* Fill in physical addresses for each interface */ for (ent = addrs; ent != NULL; ent = ent->ifa_next) { if (!((ent->ifa_flags & IFF_UP) && (ent->ifa_flags & IFF_RUNNING)) || (ent->ifa_addr == NULL) || (ent->ifa_addr->sa_family != PF_PACKET)) {

This test will succeed (and print the following bessage) because sa_family is indeed not PF_PACKET, but rather (should be) PF_INET....
  fprintf(stderr, "INFO name %s is not UP/RUNNING/PF_PACKET, or has NULL ifa_addr\n", ent->ifa_name);
  fprintf(stderr, "  ent->ifa_flags=0x%08x IFF_UP=0x%08x IFF_RUNNING=0x%08x\n", ent->ifa_flags, IFF_UP, IFF_RUNNING);
  fprintf(stderr, "  ent->ifa_addr=%p\n", ent->ifa_addr);
  fprintf(stderr, "  ent->ifa_addr->sa_family=0x%08x PF_PACKET=0x%08x\n", ent->ifa_addr==NULL? -1: ent->ifa_addr->sa_family, PF_PACKET);

  sleep(0);
  continue;
}
The output is like:

/---------------------------/ INFO name lo0 is not UP/RUNNING/PF_PACKET, or has NULL ifa_addr ent->ifa_flags=0x00011049 IFF_UP=0x00000001 IFF_RUNNING=0x00000040 ent->ifa_addr=0xffffa0000253bc40 ent->ifa_addr->sa_family=0x00000000 PF_PACKET=0x00000011 INFO name eth0 is not UP/RUNNING/PF_PACKET, or has NULL ifa_addr ent->ifa_flags=0x00011043 IFF_UP=0x00000001 IFF_RUNNING=0x00000040

ent->ifa_addr=0xffffa0000253bc50 ent->ifa_addr->sa_family=0x00000000 PF_PACKET=0x00000011

So eth0 is correctly up and running, but it has a sa_family=0... This is not what I'm seeing (without Benoit's patches) - I'm seeing sa_family == AF_INET (i.e., 2).

So nodjs code fails due to ent->ifa_addr->sa_family=0.

2) Let me remove (ent->ifa_addr->sa_family != PF_PACKET) check, and output is:

sudo ./scripts/run.py -nvd -e '/tst/tst-getifaddrs.so; /cli/cli.so' ... /---------------------------/ TTRT lo0 mac=00:00:00:00:00:00 TTRT eth0 mac=52:00:00:00:70:bc

BUT the rest of the world sees eth0 mac 52:54:00:12:34:56.

Justin, your test code does:

   sll = (struct sockaddr_ll*)ent->ifa_addr;

But "sockaddr_ll" (ll = "link level", i.e., L2) is not the address format of all interface types - it is sadly only the address format of the PF_PACKET interfaces. The PF_INET interfaces use "sockaddr_in", which does NOT have a MAC address in its end...

So your code, also on Linux (and I verified this), will only print the MAC address if used on a PF_PACKET interface, not on a PF_INET interface. But unfortunately, on OSv and its getifaddrs we do not return any PF_PACKET interface :-(

So, does the OpenMPI code actually expect to find a PF_PACKET interface in the list and use that to find the mac address? Can you please show us a snippet of this code?

If so, Benoit's patch might not be good enough, because it returns the MAC address for the normal PF_INET interfaces (something which Linux does not do) but doesn't return any PF_PACKET interface which the code might be looking for.

This ifgetaddrs() API really sucks :-(

nyh commented 8 years ago

A good discussion on PF_PACKET interfaces in ifgetaddrs(), mac addresses, and the SIOCGIFHWADDR ioctl which I mentioned to Benoit yesterday:

http://stackoverflow.com/questions/6762766/mac-address-with-getifaddrs

miha-plesko commented 8 years ago

I've only understood basics of the discussion above, but what is the final outcome? Is there a way to fix/avoid this MAC issue or is it a dead end for the NodeJS integration?

I really hope that there is a solution :)

justinc1 commented 8 years ago

@nyh I just wanted to see if only check is to strict. Thank you for pointing out that different PF_* family means requires different interpretation of binary data. I didn't event want to look at that (and than missed the 'huge tiny detail'), I just wanted to see if NodeJS should work in same way as it does on Linux.

What would be best/easiest way to get nodeJS working?

make OSv return also PF_PACKET ifaces?
patch app/node, so that it will use PF_INET instead?
something else maybe?

nyh commented 8 years ago

@miha-plesko the question is what nodejs expects the output of getifaddrs() to contain - if it expects to find a PF_PACKET interface, we need to return those (and use sockaddr_ll for their addresses), and it's not enough that we have other ways to get the mac address. Does nodejs expect those PF_PACKET interfaces? Can you show us the code snippet which uses getifaddrs() and fails?

If needed, it should be pretty easy to modify getifaddrs() to return PF_PACKET interfaces even if our lower levels do not support them - just output for every IF_INET interface which has a mac address an additional PF_PACKET which has this mac address in the sockaddr_ll address of this interface.

By the way we don't even have a header file defining sockaddr_ll. We can either just assume the compilation system has <linux/if_packet.h> (ugly and problematic) or perhaps define it in a new linux/if_packet.h header file in include/api.

justinc1 commented 8 years ago

The attached snippet tst-getifaddrs.c.txt (comment num 2) is almost exact copy-paste from nodeJS source. See also https://github.com/nodejs/node/blob/master/deps/uv/src/unix/linux-core.c#L862 (OSv apps uses different branch). Once that snippet works, I expect that nodeJS will too (that's why I stole test code from nodeJS).

miha-plesko commented 8 years ago

@nyh should you need JavaScript code snippet, here it is:

// IP ADDRESS
console.log('My IP is: ' + require("ip").address() );

// MAC ADDRESS
require('macaddress').one(function (err, mac) {
    console.log("My MAC is: %s", mac);
});

Below please find tracedown from JavaScript thru NodeJS down to OSv source code.

Tracing javascript macaddress library

Both require("ip") and require("macaddress") libraries come down to call JavaScript's core os.networkInterfaces function.

Tracing NodeJS source

Then os.networkInterfaces JavaScript function maps to NodeJS C++ GetInterfaceAddresses function which makes use of uv_interface_addresses.

Tracing OSv source

Function uv_interface_addresses (from NodeJS source) uses OSv's getifaddrs() function and that, I believe, is where incorrect data is provided.

nyh commented 8 years ago

Thanks @miha-plesko for the "uv_interface_addresses" reference. Looking at this code, I see that it expects that getifaddrs() return:

A non-AF_PACKET (AF_INET or AF_INET6 ) interface for each real interface.
Each one of these AF_INET interfaces has a sockaddr_in address, with an IP address (no mac).
For each of the real interfaces returned by geifaddrs(), it should return another AF_PACKET interface with a sockaddr_ll address which includes a MAC address. Importantly, the user code does not rely on the order - we can put all these PF_PACKET interfaces at the end, or put each PF_PACKET interface right after the corresponding AF_INET one - whatever is convenient for us in the getifaddrs() code.

1 and 2 are already done, all that remains is 3, and it looks pretty simple and close to what Benoit already did - just don't put the mac address on the same AF_INET interface, but add another one.

miha-plesko commented 8 years ago

That's good news, if you prepare a patch, I'll be happy to test :D

miha-plesko commented 8 years ago

Hi @nyh , is there any update regarding the patch? :)

benoit-canet commented 8 years ago

Hi Miha,

I will iterate on it today.

Best regards

Benoît

On Thu, Aug 25, 2016 at 8:51 AM, Miha Pleško notifications@github.com wrote:

Hi @nyh https://github.com/nyh , is there any update regarding the patch? :)

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/cloudius-systems/osv/issues/787#issuecomment-242296232, or mute the thread https://github.com/notifications/unsubscribe-auth/AAA5IY4DhxmE192XbyI3H8R7gpWYEWqMks5qjTtUgaJpZM4JlWmn .

cloudius-systems / osv