[musl] netlink.c: missing handling of EAGAIN and EWOULDBLOCK

[musl] netlink.c: missing handling of EAGAIN and EWOULDBLOCK

[Waldek Kozaczuk]

[-- Attachment #1: Type: text/plain, Size: 920 bytes --]

Hi,

Very recently we implemented minimal rnetlink support on OSv side which
allowed us to finally switch to the netlink-based implementation of
getifaddrs() and if_nameindex().

However, I noticed that the function __netlink_enumerate() in
https://github.com/ifduyue/musl/blob/master/src/network/netlink.c uses
MSG_DONTWAIT flag when calling recv() which may fail with EAGAIN or
EWOULDBLOCK and there is no error/retry handling for that. I actually saw
both functions fail occasionally on OSv.

One way to fix is to add missing error handling. But another simpler
solution is to stop using MSG_DONTWAIT altogether and force recv() to
block. In other words, the line:

r = recv(fd, u.buf, sizeof(u.buf), MSG_DONTWAIT);

should change to:

r = recv(fd, u.buf, sizeof(u.buf), 0);

For time being we are applying a header trick on OSv side to re-define
MSG_DONTWAIT as 0 when compiling those specific musl sources.

Waldek

[-- Attachment #2: Type: text/html, Size: 5034 bytes --]

Re: [musl] netlink.c: missing handling of EAGAIN and EWOULDBLOCK

[Rich Felker]

On Mon, Jun 13, 2022 at 11:41:57AM -0400, Waldek Kozaczuk wrote:
> Hi,
> 
> Very recently we implemented minimal rnetlink support on OSv side which
> allowed us to finally switch to the netlink-based implementation of
> getifaddrs() and if_nameindex().
> 
> However, I noticed that the function __netlink_enumerate() in
> https://github.com/ifduyue/musl/blob/master/src/network/netlink.c uses
> MSG_DONTWAIT flag when calling recv() which may fail with EAGAIN or
> EWOULDBLOCK and there is no error/retry handling for that. I actually saw
> both functions fail occasionally on OSv.
> 
> One way to fix is to add missing error handling. But another simpler
> solution is to stop using MSG_DONTWAIT altogether and force recv() to
> block. In other words, the line:
> 
> r = recv(fd, u.buf, sizeof(u.buf), MSG_DONTWAIT);
> 
> should change to:
> 
> r = recv(fd, u.buf, sizeof(u.buf), 0);
> 
> For time being we are applying a header trick on OSv side to re-define
> MSG_DONTWAIT as 0 when compiling those specific musl sources.

Thanks! I'll try to track this down. One concern is that I'm not sure
how MSG_DONTWAIT is supposed to interact with "short reads" -- is it
needed (for netlink) to prevent blocking when some data has been read
but there is still buffer space for more?

On a related issue, I'm pretty sure the netlink API doesn't allow for
partial reads with some data remaining buffered on the kernel side,
but we should probably verify that too.

Rich

Re: [musl] netlink.c: missing handling of EAGAIN and EWOULDBLOCK

[Waldek Kozaczuk]

[-- Attachment #1: Type: text/plain, Size: 2292 bytes --]

Hi,

I appreciate your reply.

Unfortunately, I am not an expert on netlink but another thing I have found
is that for comparison Golang uses a 0 flags argument when reading from
netlink socket -
https://github.com/golang/go/blob/master/src/syscall/netlink_linux.go#L79.

On a side note, I could not replicate the EAGAIN errors in Linux so you may
be right that Linux has some special handling of recv() for netlink sockets
which OSv might be missing. But I could not find anything in the
official netlink docs - https://man7.org/linux/man-pages/man7/netlink.7.html
and https://man7.org/linux/man-pages/man7/rtnetlink.7.html. And if that is
undocumented, shall we rely on it implicitly?

Waldek

On Mon, Jun 13, 2022 at 1:08 PM Rich Felker <dalias@libc.org> wrote:

> On Mon, Jun 13, 2022 at 11:41:57AM -0400, Waldek Kozaczuk wrote:
> > Hi,
> >
> > Very recently we implemented minimal rnetlink support on OSv side which
> > allowed us to finally switch to the netlink-based implementation of
> > getifaddrs() and if_nameindex().
> >
> > However, I noticed that the function __netlink_enumerate() in
> > https://github.com/ifduyue/musl/blob/master/src/network/netlink.c uses
> > MSG_DONTWAIT flag when calling recv() which may fail with EAGAIN or
> > EWOULDBLOCK and there is no error/retry handling for that. I actually saw
> > both functions fail occasionally on OSv.
> >
> > One way to fix is to add missing error handling. But another simpler
> > solution is to stop using MSG_DONTWAIT altogether and force recv() to
> > block. In other words, the line:
> >
> > r = recv(fd, u.buf, sizeof(u.buf), MSG_DONTWAIT);
> >
> > should change to:
> >
> > r = recv(fd, u.buf, sizeof(u.buf), 0);
> >
> > For time being we are applying a header trick on OSv side to re-define
> > MSG_DONTWAIT as 0 when compiling those specific musl sources.
>
> Thanks! I'll try to track this down. One concern is that I'm not sure
> how MSG_DONTWAIT is supposed to interact with "short reads" -- is it
> needed (for netlink) to prevent blocking when some data has been read
> but there is still buffer space for more?
>
> On a related issue, I'm pretty sure the netlink API doesn't allow for
> partial reads with some data remaining buffered on the kernel side,
> but we should probably verify that too.
>
> Rich
>

[-- Attachment #2: Type: text/html, Size: 3219 bytes --]

Re: [musl] netlink.c: missing handling of EAGAIN and EWOULDBLOCK

[Timo Teras]

On Mon, 13 Jun 2022 13:08:49 -0400
Rich Felker <dalias@libc.org> wrote:

> On Mon, Jun 13, 2022 at 11:41:57AM -0400, Waldek Kozaczuk wrote:
> > Very recently we implemented minimal rnetlink support on OSv side
> > which allowed us to finally switch to the netlink-based
> > implementation of getifaddrs() and if_nameindex().
> > 
> > However, I noticed that the function __netlink_enumerate() in
> > https://github.com/ifduyue/musl/blob/master/src/network/netlink.c
> > uses MSG_DONTWAIT flag when calling recv() which may fail with
> > EAGAIN or EWOULDBLOCK and there is no error/retry handling for
> > that. I actually saw both functions fail occasionally on OSv.

In linux the kernel generates the responses during recv() syscall, which
guarantees that there is no EAGAIN or EWOULDBLOCK unless the operation
has completed and there really is no longer any response data left.

IIRC, I added the MSG_DONTWAIT just to be protective against bad
implementation to not block indefinitely if NLMSG_DONE/NLMSG_ERROR is
not received or parsed properly. This could happen e.g. if the
assumption about buffer size no longer is true. But seems 8kB is now
documented as upper buffer size universally.

> > One way to fix is to add missing error handling. But another simpler
> > solution is to stop using MSG_DONTWAIT altogether and force recv()
> > to block. In other words, the line:
> > 
> > r = recv(fd, u.buf, sizeof(u.buf), MSG_DONTWAIT);
> > 
> > should change to:
> > 
> > r = recv(fd, u.buf, sizeof(u.buf), 0);

This should work equally well on Linux if the kernel response is parsed
correctly and contains a terminating message.

> > For time being we are applying a header trick on OSv side to
> > re-define MSG_DONTWAIT as 0 when compiling those specific musl
> > sources.  
> 
> Thanks! I'll try to track this down. One concern is that I'm not sure
> how MSG_DONTWAIT is supposed to interact with "short reads" -- is it
> needed (for netlink) to prevent blocking when some data has been read
> but there is still buffer space for more?
>
> On a related issue, I'm pretty sure the netlink API doesn't allow for
> partial reads with some data remaining buffered on the kernel side,
> but we should probably verify that too.

The netlink socket is datagram-oriented. The kernel has special
measures on how the datagrams are generated. Currently NLM_F_DUMP
requests generate maximum size of NLMSG_GOODSIZE (capped to 8kB by code
and docs) to make sure no super large buffers are needed. And as
mentioned, the current implementation generates additional response
datagrams during the syscall if possible. So the socket buffer size
does not really effect things here. (The netlink socket buffer size
affects only sockets listening for events where the messages are really
buffered.)

Another way to detect the wrong-sized buffer is to use MSG_TRUNC and
make sure the returned value is not larger than buffer size. If that
happens, then the buffer size is not enough.

But given the current man page, and stability of the Linux
implementation. I think we can assume the 8kB buffer size and current
code is good.

I would not have objection on the suggested change.

However, if we do support alternative implementations, it might be good
to document the assumptions made. I wonder if the buffer size check is
needed in case the alternate implementation used something different?

Timo