Re: [PATCH 1/4] netlink: Use 8 KiB * netlink message header size as response buffer

[David Gibson <david@gibson.dropbear.id.au>]

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

On Wed, Mar 08, 2023 at 08:35:13AM +0100, Stefano Brivio wrote:
> ...instead of BUFSIZ. On musl, BUFSIZ is 1024, so we'll typically
> truncate the response to the request we send in nl_link(). It's
> usually 8192 or more with glibc.
> 
> There doesn't seem to be any macro defining the rtnetlink maximum
> message size, and iproute2 just hardcodes 1024 * 1024 for the receive
> buffer, but the example in netlink(7) makes somewhat sense, looking
> at the kernel implementation.
> 
> It's not very clean, but we're very unlikely to hit that limit,
> and if we do, we'll find out painlessly, because NLA_OK() will tell
> us right away.
> 
> Reported-by: Chris Kuhn <kuhnchris+passt@kuhnchris.eu>
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

I think controlling buffer sizes ourselves explicitly makes more
sense, even if it weren't for the musl issues.

> ---
>  netlink.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/netlink.c b/netlink.c
> index 8f785ca..0e0be4f 100644
> --- a/netlink.c
> +++ b/netlink.c
> @@ -34,6 +34,8 @@
>  #include "log.h"
>  #include "netlink.h"
>  
> +#define NLBUFSIZ	(8192 * sizeof(struct nlmsghdr)) /* See netlink(7) */
> +
>  /* Socket in init, in target namespace, sequence (just needs to be monotonic) */
>  static int nl_sock	= -1;
>  static int nl_sock_ns	= -1;
> @@ -105,7 +107,7 @@ fail:
>  static int nl_req(int ns, char *buf, const void *req, ssize_t len)
>  {
>  	int s = ns ? nl_sock_ns : nl_sock, done = 0;
> -	char flush[BUFSIZ];
> +	char flush[NLBUFSIZ];
>  	ssize_t n;
>  
>  	while (!done && (n = recv(s, flush, sizeof(flush), MSG_DONTWAIT)) > 0) {
> @@ -121,7 +123,8 @@ static int nl_req(int ns, char *buf, const void *req, ssize_t len)
>  		}
>  	}
>  
> -	if ((send(s, req, len, 0) < len) || (len = recv(s, buf, BUFSIZ, 0)) < 0)
> +	if ((send(s, req, len, 0) < len) ||
> +	    (len = recv(s, buf, NLBUFSIZ, 0)) < 0)
>  		return -errno;
>  
>  	return len;
> @@ -149,7 +152,7 @@ unsigned int nl_get_ext_if(sa_family_t af)
>  	};
>  	struct nlmsghdr *nh;
>  	struct rtattr *rta;
> -	char buf[BUFSIZ];
> +	char buf[NLBUFSIZ];
>  	ssize_t n;
>  	size_t na;
>  
> @@ -227,7 +230,7 @@ void nl_route(int ns, unsigned int ifi, sa_family_t af, void *gw)
>  	struct nlmsghdr *nh;
>  	struct rtattr *rta;
>  	struct rtmsg *rtm;
> -	char buf[BUFSIZ];
> +	char buf[NLBUFSIZ];
>  	ssize_t n;
>  	size_t na;
>  
> @@ -336,7 +339,7 @@ void nl_addr(int ns, unsigned int ifi, sa_family_t af,
>  	struct ifaddrmsg *ifa;
>  	struct nlmsghdr *nh;
>  	struct rtattr *rta;
> -	char buf[BUFSIZ];
> +	char buf[NLBUFSIZ];
>  	ssize_t n;
>  	size_t na;
>  
> @@ -446,7 +449,7 @@ void nl_link(int ns, unsigned int ifi, void *mac, int up, int mtu)
>  	struct ifinfomsg *ifm;
>  	struct nlmsghdr *nh;
>  	struct rtattr *rta;
> -	char buf[BUFSIZ];
> +	char buf[NLBUFSIZ];
>  	ssize_t n;
>  	size_t na;
>  

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]