[-- 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 --]
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 #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 --]
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