3729 getifaddrs must learn to stop worrying and love the other address families

Reviews
Diff

- Download Diff

Information
Submitter:	Sebastian Wiedenroth
Repository:	illumos-gate
Branch:
Bugs:
Depends On:
Reviewers
Groups:	general
People:

Description

Presently getifaddrs enumerates network interface addresses only from AF_INET and AF_INET6.

This is contrary to the expectations of a small set of software (e.g. dhcpcd, olsrd) which expects this function to deal also in AF_LINK.
With this change getifaddrs() also returns AF_LINK entries, supplying the MAC address in 'ifa_addr' and a reasonably filled 'if_data_t' in 'ifa_data'.

This should be sufficient for most applications that are interested in things like the MAC address or the MTU.

Testing Done

Booted OI with this change, ran a small tool that prints the getifaddrs() details.
Ran the test tool with and without the environment variable GETIFADDRS_MAY_RETURN_AF_LINK=1 set.
Rebuild the test tool and retried.

Ran with libumem and ::findleaks on the test tool (that also calls freeifaddrs()) in all four combinations.

Issues

1
23
6
All Issues: 30

Description	From	Last Updated
I'd still remove "BUGS", but mention under notes that AF_INET, AF_INET6, and AF_LINK are returned, and that any other address ...	Dan McDonald	May 26, 2017, 7:02 p.m.
Probably "was not" and "door allocated"?	Yuri Pankov	May 26, 2017, 12:23 a.m.
Where this number comes from?	Yuri Pankov	May 26, 2017, 12:23 a.m.
Use for (;;) instead.	Yuri Pankov	May 26, 2017, 7:01 p.m.
Remove extra empty line.	Yuri Pankov	May 26, 2017, 7:01 p.m.
Move declarations to the start of the code block.	Yuri Pankov	May 26, 2017, 7:02 p.m.
This should probably be static.	Robert Mustacchi	May 27, 2017, 12:14 p.m.
It'd be helpful to describe the error semantics here. Notably that -1 / errno is used if the door call ...	Robert Mustacchi	May 27, 2017, 6:33 p.m.
Same static note as earlier.	Robert Mustacchi	May 27, 2017, 12:22 p.m.
Is there a reason this is cast to void?	Robert Mustacchi	May 28, 2017, 4:30 p.m.
Same static note as earlier.	Robert Mustacchi	May 27, 2017, 12:29 p.m.
As these are private structures unlike the door_arg_t, I recommend always running a bzero first. This way if the structure ...	Robert Mustacchi	May 27, 2017, 12:29 p.m.
If you're going to return a boolean, why not make this return a boolean_t?	Robert Mustacchi	May 28, 2017, 4:30 p.m.
Same static note as earlier.	Robert Mustacchi	May 27, 2017, 12:32 p.m.
As these are private structures unlike the door_arg_t, I recommend always running a bzero first. This way if the structure ...	Robert Mustacchi	May 27, 2017, 12:32 p.m.
If you're going to return a boolean, why not make this return a boolean_t?	Robert Mustacchi	May 27, 2017, 6:36 p.m.
The cast should be unnecessary.	Robert Mustacchi	May 27, 2017, 12:32 p.m.
While both of these structures are the same size, I'm not sure if this is safe or not. Consider the ...	Robert Mustacchi	May 27, 2017, 6:37 p.m.
So the size of the address is probably less than the size of the sockaddr_storage. Should we make sure to ...	Robert Mustacchi	May 27, 2017, 12:51 p.m.
Do other implementations prefer the IPv4 index over the IPv6?	Robert Mustacchi	May 27, 2017, 3:24 p.m.
Sorry, I missed this earlier. But this could clobber errno potentially.	Robert Mustacchi	May 27, 2017, 6:38 p.m.
In this failure case, how is errno being set?	Robert Mustacchi	May 27, 2017, 6:38 p.m.
Same errno comment here.	Robert Mustacchi	May 27, 2017, 6:38 p.m.
I would just sanity check this by actually checking for overflow and if it does, just continue or skip this ...	Robert Mustacchi	May 27, 2017, 6:39 p.m.
We currently don't use stdbool.h and use our own boolean_t instead in illumos-gate (boolean_t predates stdbool.h and I think no ...	Jason King	Oct. 23, 2017, 10:18 p.m.
Use B_FALSE	Jason King	Oct. 23, 2017, 10:18 p.m.
and B_TRUE here	Jason King	Oct. 23, 2017, 10:18 p.m.
B_TRUE	Jason King	Oct. 23, 2017, 10:18 p.m.
Why remove this line?	Dan McDonald	Nov. 13, 2017, 7:05 a.m.
Do we really need to make it global???	Yuri Pankov	Dec. 2, 2017, 12:34 a.m.

usr/src/man/man3socket/getifaddrs.3socket (Diff revision 1)

Show all issues

I'd still remove "BUGS", but mention under notes that AF_INET, AF_INET6, and AF_LINK are returned, and that any other address families (not that there are any I can think of currently) are not returned. This way, we insure proper setting of expectations.

Sebastian Wiedenroth May 25, 2017, 11:05 p.m.

I've updated the man page for the new version. Is this what you had in mind?

Description:

		Presently getifaddrs enumerates network interface addresses only from AF_INET and AF_INET6.
~		This is contrary to the expectations of a small set of software (e.g. allegedly ISC dhcpd, olsrd) which expects this function to deal also in AF_LINK.
	~	This is contrary to the expectations of a small set of software (e.g. dhcpcd, olsrd) which expects this function to deal also in AF_LINK.

		With this change getifaddrs() also returns AF_LINK entries, supplying the MAC address in 'ifa_addr' and a reasonably filled 'if_data_t' in 'ifa_data'.
		This should be sufficient for most applications that are interested in things like the MAC address or the MTU.

Diff:

Revision 2 (+265 -12)

Show changes

	usr/src/cmd/fs.d/nfs/statd/sm_proc.c
	usr/src/lib/libipadm/common/ipadm_addr.c
	usr/src/lib/libipadm/common/ipadm_if.c
	usr/src/lib/libsocket/inet/getifaddrs.c
	usr/src/man/man3socket/getifaddrs.3socket

Sebastian Wiedenroth May 26, 2017, 12:01 a.m.

thank you very much for your feedback.
I've added a few comments and will fix the other issues.

usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 2)

Show all issues

Probably "was not" and "door allocated"?

Sebastian Wiedenroth May 26, 2017, 12:01 a.m.

this was taken from dladm_door_call() in usr/src/lib/libdladm/common/libdlmgmt.c
I'll fix this here, should I also update the original source?

Yuri Pankov May 26, 2017, 12:24 a.m.

If you wish to, I didn't want to mark this as issue, just as hint.

usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 2)

Show all issues

Where this number comes from?

Sebastian Wiedenroth May 26, 2017, 12:01 a.m.

From looking at dladm_walk_macaddr() in usr/src/lib/libdladm/common/libdllink.c

Yuri Pankov May 26, 2017, 12:24 a.m.

It's strange that we don't have a define for this.

usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 2)

How about the following given that socket() calls really shouldn't fail, so we won't get in that situation too often? Just trying to reduce repeating stuff:
if ((sock4 = socket(AF_INET, SOCK_DGRAM, 0)) < 0 ||
    (sock6 = socket(AF_INET6, SOCK_DGRAM, 0)) < 0 ||
    (door_fd = open(DLMGMT_DOOR, O_RDONLY)) < 0 ||
    (dld_fd = open(DLD_CONTROL_DEV, O_RDWR)) < 0) {
        err = errno;
        close(sock4);
        close(sock6);
        close(door_fd);
        errno = err;
        return (-1);
}

Sebastian Wiedenroth May 26, 2017, 12:01 a.m.

that looks indeed much better. I wasn't sure if it was safe to call close() like that. I guess I'll need to make sure the sockets are initialized to something invalid like -1?

Yuri Pankov May 26, 2017, 12:24 a.m.

Yeah, hopefully others won't find this bad/ugly :-) and even if libsocket is not linted, let's cast the return values of close() to (void), showing that we really don't care about its return value.

usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 2)

Show all issues

Use for (;;) instead.

Vitaliy Gusev May 26, 2017, 4:15 p.m.

Yuri, why for(;;) is better  than while (1) ?

Yuri Pankov May 26, 2017, 4:23 p.m.

Even if we don't lint libsocket, just to be consistent with other parts of the code where lint would whine about using constant condition. And a lot of style guides I did check (from time to time) do say "Forever loops are done with for's, not while's.".

Vitaliy Gusev May 26, 2017, 5 p.m.

Never heard about that. It looks as overhead styles :) Consisetency is good reason. Thanks.

usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 2)

Show all issues

Remove extra empty line.

usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 2)

Show all issues

Move declarations to the start of the code block.

usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 2)

What if we have both v4 and v6 plumbed? Do we even want this information for AF_LINK?

Sebastian Wiedenroth May 26, 2017, 12:01 a.m.

It does not matter if we check with v4 or v6 because the information should be the same.
The interface index is useful to match an ip address to a mac address.

Yuri Pankov May 26, 2017, 12:24 a.m.

My point is that if we can't rely on this information to be available always, it's not that useful.

Sebastian Wiedenroth May 26, 2017, 3:30 p.m.

In the case where we have an ip address and want to match it to a mac adress it is available, so it is there when we need it.

But I've looked a bit closer at this again and indeed the metric is NOT guaranteed to be the same. So I'll adjust it to not provide it for AF_LINK entries.

Change Summary:

feedback from yuri

Diff:

Revision 3 (+254 -26)

Show changes

	usr/src/cmd/fs.d/nfs/statd/sm_proc.c
	usr/src/lib/libipadm/common/ipadm_addr.c
	usr/src/lib/libipadm/common/ipadm_if.c
	usr/src/lib/libsocket/inet/getifaddrs.c
	usr/src/man/man3socket/getifaddrs.3socket

Ship it!

This looks good to me, but you really should have Dan review this.

In general, I think this looks good. However, there are a bunch of nits and the like below.

usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 3)

Show all issues

This should probably be static.

usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 3)

Show all issues

It'd be helpful to describe the error semantics here. Notably that -1 / errno is used if the door call fails and that otherwise a positive error value is returned.

Honestly, it may more sense to distinguish between the success of the door call and instead have a separate argument to take the door return value semantically to avoid the overloading.

Sebastian Wiedenroth May 27, 2017, 12:56 p.m.

As there is already rbuf I've changed the return values around a bit. I hope this makes things cleaner. Let me know if this is what you had in mind and if I can improve things further.

Robert Mustacchi May 27, 2017, 3:34 p.m.

Sure, this works as a way to do this, thanks!

usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 3)

Show all issues

Same static note as earlier.

usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 3)

Show all issues

Is there a reason this is cast to void?

Sebastian Wiedenroth May 27, 2017, 12:56 p.m.

I ran lint manually and got:

warning: function returns value which is always ignored: munmap (E_FUNC_RET_ALWAYS_IGNOR2)
Not sure how I can handle that better or if it should be ignored. Happy to drop the cast if that is the preferred style.

Robert Mustacchi May 27, 2017, 3:34 p.m.

In asking this, I more so meant why are we ignoring the return value. If munmap returns EINVAL what does that mean? Does that mean there was a programmer error from us or that something is really wrong with the system?

Sebastian Wiedenroth May 27, 2017, 6:40 p.m.

quote from door_call(3C):
If the results of a door invocation exceed the size of the buffer
specified by rsize, the system automatically allocates  a new buffer in
the caller's address space  and updates the rbuf and rsize members to
reflect this location. In this case, the caller is  responsible for
reclaiming this area using munmap(rbuf, rsize) when the buffer is no
longer required.  See munmap(2).


So if something is wrong the data we have came from the door_call().

I don't think any input from us should cause door_call() to return a buffer that we'd have to cleanup but can't.
I've looked through grok and nearly all munmap calls have their return values casted away.

There are a few in testcases that have asserts and a few others that actually do some handling like printing a message.

Robert Mustacchi May 28, 2017, 4:30 p.m.

OK, fair enough. Whenver I see things that may return errors cast to void, I always want to ask. I think you're right that hopefully door_call will put something right there. If it did fail, I'm not sure if we'd want to drive on, but the much more damaging case of unmapping the wrong thing we can't tell anyways so this is probably fine.

usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 3)

Show all issues

Same static note as earlier.

usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 3)

Show all issues

As these are private structures unlike the door_arg_t, I recommend always running a bzero first. This way if the structure changes and someone forgets to update this, we're less likely to send stack garbage.

usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 3)

Show all issues

If you're going to return a boolean, why not make this return a boolean_t?

Sebastian Wiedenroth May 27, 2017, 12:56 p.m.

I've changed the return paths a bit now to explicitly return -1 or 0, but could still make it boolean_t if you think that is better.

Robert Mustacchi May 27, 2017, 3:34 p.m.

The 0 and -1/errno pattern is fine. Though I made a note about the errno consistency bit on those. Do you want to preserve the return value from dlmgmtd as the errno in those cases? Though I know it's just discarded anyways.

Sebastian Wiedenroth May 27, 2017, 6:40 p.m.

The value in lr_err can actually be used directly used as errno, so this was easy. The latest version should address this.

usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 3)

Show all issues

Same static note as earlier.

usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 3)

Show all issues

As these are private structures unlike the door_arg_t, I recommend always running a bzero first. This way if the structure changes and someone forgets to update this, we're less likely to send stack garbage.

usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 3)

Show all issues

If you're going to return a boolean, why not make this return a boolean_t?

Sebastian Wiedenroth May 27, 2017, 12:56 p.m.

I've changed the return paths a bit now to explicitly return -1 or 0, but could still make it boolean_t if you think that is better.

Robert Mustacchi May 27, 2017, 3:34 p.m.
```
See other notes.
```

usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 3)

Show all issues

The cast should be unnecessary.

usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 3)

Show all issues

While both of these structures are the same size, I'm not sure if this is safe or not. Consider the case where a bug causes dlmgmtd not to null terminate the resulting array. In that case we'd end up not null terminating lifr_name.

Do consumers of this structure assume that the name is always null terminated? Based on what I see in the manual they do.

We also may not want to bake the assumption that they're the same into this.

Sebastian Wiedenroth May 27, 2017, 12:56 p.m.

I've changed it to strlcpy() and also moved it a bit closer to the actual ioctl call.

Can you explain a bit more why we should not assume that they're the same?
I use it to fill in the ip interface index, but during the review process this code does seem to cause a bit of confusion.
While I understand that the mapping between the links and ip is a bit odd I still think the data would be useful.
So I wonder how I could structure that better or if my understanding is just too wrong and that part of the code should be dropped.

Robert Mustacchi May 27, 2017, 3:34 p.m.

In this case I left out the operative word size. I was just saying that we didn't want to assume that the dlmgmtd size (in the door call) would always be the same as the buffer size.

Strictly speaking the IP interface on top of a datalink can have a different from the datalink from the kernel's perspective, but that's impossible to do with ifconfig / ipadm, etc. so I'm not sure if that case matters.

Sebastian Wiedenroth May 27, 2017, 6:40 p.m.
```
ah ok, checking for overflow is now done.
```

usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 3)

Show all issues

So the size of the address is probably less than the size of the sockaddr_storage. Should we make sure to zero the allocation to make sure we're not leaking arbitrary data into the buffer?

usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 3)

Show all issues

Do other implementations prefer the IPv4 index over the IPv6?

Sebastian Wiedenroth May 27, 2017, 12:56 p.m.

This is not much of a preference. The returned index will be the same if both IPv4 and IPv6 are on the same link. We just have to make sure we try both in case it is IPv4-only or IPv6-only.

Robert Mustacchi May 27, 2017, 3:34 p.m.

Ah, gotcha. I wasn't sure if they had the same index or not. Thanks!

Change Summary:

feedback from robert

Diff:

Revision 4 (+273 -26)

Show changes

	usr/src/cmd/fs.d/nfs/statd/sm_proc.c
	usr/src/lib/libipadm/common/ipadm_addr.c
	usr/src/lib/libipadm/common/ipadm_if.c
	usr/src/lib/libsocket/inet/getifaddrs.c
	usr/src/man/man3socket/getifaddrs.3socket

usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 4)

Show all issues

Sorry, I missed this earlier. But this could clobber errno potentially.

usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 4)

Show all issues

In this failure case, how is errno being set?

Sebastian Wiedenroth May 27, 2017, 6:39 p.m.

We can use lr_err for this, done in the latest version.

usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 4)

Show all issues

Same errno comment here.

usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 4)

Show all issues

I would just sanity check this by actually checking for overflow and if it does, just continue or skip this bit of information.

Change Summary:

errno changes

Diff:

Revision 5 (+282 -26)

Show changes

	usr/src/cmd/fs.d/nfs/statd/sm_proc.c
	usr/src/lib/libipadm/common/ipadm_addr.c
	usr/src/lib/libipadm/common/ipadm_if.c
	usr/src/lib/libsocket/inet/getifaddrs.c
	usr/src/man/man3socket/getifaddrs.3socket

Ship it!

Ship It!

usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 5)

Tiny cstyle nit. A two-or-more-line if clause should have braces for its code, even if it's one line.

Ship it!

The above cstyle nit is just that... a nit. Otherwise, I'm happy.

Status: Closed (submitted)

Status: Re-opened

Change Summary:

added new symbol to work around libuv bug

Testing Done:

~		Booted OmniOS with this change, ran a small tool that prints the getifaddrs() details.
~		Ran with libumem and ::findleaks on the test tool (that also calls freeifaddrs()) was happy.
	~	Booted OI with this change, ran a small tool that prints the getifaddrs() details.
	~	Ran the test tool with and without the environment variable GETIFADDRS_MAY_RETURN_AF_LINK=1 set.
	+	Rebuild the test tool and retried.
	+
	+	Ran with libumem and ::findleaks on the test tool (that also calls freeifaddrs()) in all four combinations.

Diff:

Revision 6 (+357 -34)

Show changes

	usr/src/cmd/fs.d/nfs/statd/sm_proc.c
	usr/src/head/ifaddrs.h
	usr/src/lib/libipadm/common/ipadm_addr.c
	usr/src/lib/libipadm/common/ipadm_if.c
	usr/src/lib/libsocket/common/mapfile-vers
	usr/src/lib/libsocket/inet/getifaddrs.c
	usr/src/man/man3socket/getifaddrs.3socket

usr/src/lib/libsocket/inet/getifaddrs.c (Diff revisions 5 - 6)

Show all issues

We currently don't use stdbool.h and use our own boolean_t instead in illumos-gate (boolean_t predates stdbool.h and I think no one's wanted to try to convert everything nor leave things in a mixed state).

usr/src/lib/libsocket/inet/getifaddrs.c (Diff revisions 5 - 6)

Show all issues

Use B_FALSE

usr/src/lib/libsocket/inet/getifaddrs.c (Diff revisions 5 - 6)

Show all issues

and B_TRUE here

usr/src/lib/libsocket/inet/getifaddrs.c (Diff revisions 5 - 6)

Show all issues

B_TRUE

Change Summary:

use illumos boolean_t

Diff:

Revision 7 (+356 -34)

Show changes

	usr/src/cmd/fs.d/nfs/statd/sm_proc.c
	usr/src/head/ifaddrs.h
	usr/src/lib/libipadm/common/ipadm_addr.c
	usr/src/lib/libipadm/common/ipadm_if.c
	usr/src/lib/libsocket/common/mapfile-vers
	usr/src/lib/libsocket/inet/getifaddrs.c
	usr/src/man/man3socket/getifaddrs.3socket

Ship it!

Ship It!

usr/src/lib/libsocket/inet/getifaddrs.c (Diff revisions 5 - 7)

Show all issues

Why remove this line?

Sebastian Wiedenroth Nov. 11, 2017, 9:09 p.m.

This was a leftover that was missed from an even earlier version which used ioctls and ip instead of doors and datalinks.
For links those ip flags don't make much sense (they are per ip address, not per link), so they should be zero.

Ship it!

Ship It!

Fix it!

usr/src/lib/libsocket/common/mapfile-vers (Diff revision 7)

Show all issues

Do we really need to make it global???

Yuri Pankov Dec. 2, 2017, 12:38 a.m.
```
Sorry, I mean public, of course.
```

Sebastian Wiedenroth Aug. 18, 2018, 5:56 p.m.

I'm not sure.
I've tried finding a definite answer but only got more uncertain.
If this should be public, should it have two underscores as a start prefix? So even it is public it is marked as a internal thing? Where can I find rules/standards about this?
If it should not be public, what happens to software that is build now and links against it? It will always need it to be there, right?

Ship it!

Ship It!

You have a pending review.

Review Board 2.5.16

Screenshots

Files

Issues

Description:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Status: Closed (submitted)

Status: Re-opened

Change Summary:

Testing Done:

Diff:

Change Summary:

Diff: