[PATCH alternative v2] ifplugd: fix netlink recv

[Tito]

On Tuesday 09 July 2013 18:07:17 you wrote:
> On Tue, Jul 09, 2013 at 04:30:40PM +0200, Tito wrote:
> > On Tuesday 09 July 2013 15:08:50 Johannes Stezenbach wrote:
> > > Without doing research on it, my assumption is that
> > > in older kernels the message was smaller, and now it is
> > > larger, and in future kernels the size might increase even more.
> > > and it varies with the type of the interface.
> > > And the point of using the netlink protocol is that messages
> > > can be extended with additional attributes without breaking
> > > backwards compatibility.
> > > Or is there some max size guaranteed for NETLINK_ROUTE messages
> > > in group RTMGRP_LINK?  If not, then the code needs to
> > > be able to handle the max size of a netlink message.
> > >
> > 
> > Looking at the __kernel__  of part netlink.h it seems to me
> > that the max size for a netlink message is 8k
> > 
> >   *      skb should fit one page. This choice is good for headerless malloc.
> >   *      But we should limit to 8K so that userspace does not have to
> >   *      use enormous buffer sizes on recvmsg() calls just to avoid
> >   *      MSG_TRUNC when PAGE_SIZE is very large.
> >   */
> >   #if PAGE_SIZE < 8192UL
> >   #define NLMSG_GOODSIZE  SKB_WITH_OVERHEAD(PAGE_SIZE)
> >   #else
> >   #define NLMSG_GOODSIZE  SKB_WITH_OVERHEAD(8192UL)
> >   #endif
> >   
> >   #define NLMSG_DEFAULT_SIZE (NLMSG_GOODSIZE - NLMSG_HDRLEN)
> 
> That matches networking/libiproute/libnetlink.c which uses
>         char *buf = xmalloc(8*1024); /* avoid big stack buffer */
> 
> (not sure what is the advantage of using xmalloc() vs. stack)
> 
> > also looking at the ifplugd man page
> > 
> > "ifplugd [options]
> > Description
> > ifplugd is a daemon which will automatically configure your ethernet device when
> > a cable is plugged in and automatically unconfigure it if the cable is pulled.
> > This is useful on laptops with on-board network adapters, since it will only
> > configure the interface when a cable is really connected.  "
> > 
> > it seems to me that ifplugd is to be used only with ethernet devices
> > so maybe a 1024 buffer is enough for them but not for br* devices?
> 
> Maybe br0 is an artificial test case, but IMHO ifplugd should
> work on all interfaces, e.g. monitor when wlan0 devices appear
> when USB dongle is plugged in, or reconfigured between AP
> and client mode etc.  Also you missed the point that it doesn't
> matter what the currect kernel does, we want something that
> is guaranteed to work with future kernels that might increase
> the message size.
> 
> 
> > Also while reading about this netlink stuff I noticed the existence of
> > multipart messages, is this our case?
> > 
> > Did you check the size of the offending netlink message just for reference?
> > 
> > Also checking the kernel source for the module of br devices could be an option.
> > 
> > I'm wondering If it would be possible to malloc the buffer on demand like:
> > 
> > while (1) {
> > 	read_just_the_nl_msg_hdr_first
> > 	s = nlmsg_len - size_of_header
> > 	buf = malloc(s)
> > 	read_rest_of_msg
> > 	(concatenate messges if needed?)
> > 	do something 
> > 	....
> > 	free it
> > }
> 
> I like the simple code much better.  Let Denys decide.
> 
> 
> Thanks,
> Johannes
> 

Hi,
here v2 of the patch that mallocs a buffer the size of pagesize and not more then 8192.
I tested it and it seems to work as before on my system. Can you test if it solves your problem.

Ciao,
Tito

--- networking/ifplugd.c.orig	2013-07-08 22:51:22.748468426 +0200
+++ networking/ifplugd.c	2013-07-09 22:14:50.105451385 +0200
@@ -451,17 +451,19 @@ static smallint detect_link(void)
 static NOINLINE int check_existence_through_netlink(void)
 {
 	int iface_len;
-	char replybuf[1024];
+	/* netlink.h: limit to 8K to avoid MSG_TRUNC when PAGE_SIZE is very large.*/
+	int size = MIN(sysconf(_SC_PAGESIZE), 8192);
+	char *replybuf = xmalloc(size);
 
 	iface_len = strlen(G.iface);
 	while (1) {
 		struct nlmsghdr *mhdr;
 		ssize_t bytes;
 
-		bytes = recv(netlink_fd, &replybuf, sizeof(replybuf), MSG_DONTWAIT);
+		bytes = recv(netlink_fd, replybuf, size, MSG_DONTWAIT);
 		if (bytes < 0) {
 			if (errno == EAGAIN)
-				return G.iface_exists;
+				break;
 			if (errno == EINTR)
 				continue;
 
@@ -506,7 +508,8 @@ static NOINLINE int check_existence_thro
 			mhdr = NLMSG_NEXT(mhdr, bytes);
 		}
 	}
-
+	/* on error program exits so no need to free */
+	free(replybuf);
 	return G.iface_exists;
 }
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: ifplugd_v2.patch
Type: text/x-patch
Size: 1141 bytes
Desc: not available
URL: <http://lists.busybox.net/pipermail/busybox/attachments/20130709/70d56456/attachment-0001.bin>