netdev - Re: [PATCH net-next 10/15] tools: ynl: stop using mnl_cb

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <b2fd9413-afb2-4ed7-b660-2294241a8adc@6wind.com>
Date: Fri, 23 Feb 2024 16:50:03 +0100
From: Nicolas Dichtel <nicolas.dichtel@...nd.com>
To: Jakub Kicinski <kuba@...nel.org>, davem@...emloft.net
Cc: netdev@...r.kernel.org, edumazet@...gle.com, pabeni@...hat.com,
 jiri@...nulli.us, sdf@...gle.com, donald.hunter@...il.com
Subject: Re: [PATCH net-next 10/15] tools: ynl: stop using mnl_cb_run2()

Le 23/02/2024 à 00:56, Jakub Kicinski a écrit :
> There's only one set of callbacks in YNL, for netlink control
> messages, and most of them are trivial. So implement the message
> walking directly without depending on mnl_cb_run2().
> 
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>
> ---
>  tools/net/ynl/lib/ynl.c | 66 +++++++++++++++++++++++++++++------------
>  tools/net/ynl/lib/ynl.h |  1 +
>  2 files changed, 48 insertions(+), 19 deletions(-)
> 
> diff --git a/tools/net/ynl/lib/ynl.c b/tools/net/ynl/lib/ynl.c
> index c01a971c251e..0f96ce948f75 100644
> --- a/tools/net/ynl/lib/ynl.c
> +++ b/tools/net/ynl/lib/ynl.c
> @@ -255,10 +255,10 @@ ynl_ext_ack_check(struct ynl_sock *ys, const struct nlmsghdr *nlh,
>  	return MNL_CB_OK;
>  }
>  
> -static int ynl_cb_error(const struct nlmsghdr *nlh, void *data)
> +static int
> +ynl_cb_error(const struct nlmsghdr *nlh, struct ynl_parse_arg *yarg)
>  {
>  	const struct nlmsgerr *err = ynl_nlmsg_data(nlh);
> -	struct ynl_parse_arg *yarg = data;
>  	unsigned int hlen;
>  	int code;
>  
> @@ -275,9 +275,8 @@ static int ynl_cb_error(const struct nlmsghdr *nlh, void *data)
>  	return code ? MNL_CB_ERROR : MNL_CB_STOP;
>  }
>  
> -static int ynl_cb_done(const struct nlmsghdr *nlh, void *data)
> +static int ynl_cb_done(const struct nlmsghdr *nlh, struct ynl_parse_arg *yarg)
>  {
> -	struct ynl_parse_arg *yarg = data;
>  	int err;
>  
>  	err = *(int *)NLMSG_DATA(nlh);
> @@ -292,18 +291,6 @@ static int ynl_cb_done(const struct nlmsghdr *nlh, void *data)
>  	return MNL_CB_STOP;
>  }
>  
> -static int ynl_cb_noop(const struct nlmsghdr *nlh, void *data)
> -{
> -	return MNL_CB_OK;
> -}
> -
> -static mnl_cb_t ynl_cb_array[NLMSG_MIN_TYPE] = {
> -	[NLMSG_NOOP]	= ynl_cb_noop,
> -	[NLMSG_ERROR]	= ynl_cb_error,
> -	[NLMSG_DONE]	= ynl_cb_done,
> -	[NLMSG_OVERRUN]	= ynl_cb_noop,
> -};
> -
>  /* Attribute validation */
>  
>  int ynl_attr_validate(struct ynl_parse_arg *yarg, const struct nlattr *attr)
> @@ -475,14 +462,55 @@ static int ynl_cb_null(const struct nlmsghdr *nlh, void *data)
>  static int ynl_sock_read_msgs(struct ynl_parse_arg *yarg, mnl_cb_t cb)
>  {
>  	struct ynl_sock *ys = yarg->ys;
> -	ssize_t len;
> +	ssize_t len, rem;
> +	int ret;
>  
>  	len = mnl_socket_recvfrom(ys->sock, ys->rx_buf, MNL_SOCKET_BUFFER_SIZE);
>  	if (len < 0)
>  		return len;
>  
> -	return mnl_cb_run2(ys->rx_buf, len, ys->seq, ys->portid,
> -			   cb, yarg, ynl_cb_array, NLMSG_MIN_TYPE);
> +	ret = MNL_CB_STOP;
> +	for (rem = len; rem > 0;) {
Maybe this (if the nlh declaration is put above)?
for (rem = len; rem > 0; NLMSG_NEXT(nlh, rem)) {

> +		const struct nlmsghdr *nlh;
> +
> +		nlh = (struct nlmsghdr *)&ys->rx_buf[len - rem];
> +		if (!NLMSG_OK(nlh, rem)) {
> +			yerr(yarg->ys, YNL_ERROR_INV_RESP,
> +			     "Invalid message or trailing data in the response.");
> +			return MNL_CB_ERROR;
> +		}
> +
> +		if (nlh->nlmsg_flags & NLM_F_DUMP_INTR) {
> +			/* TODO: handle this better */
> +			yerr(yarg->ys, YNL_ERROR_DUMP_INTER,
> +			     "Dump interrupted / inconsistent, please retry.");
> +			return MNL_CB_ERROR;
> +		}
> +
> +		switch (nlh->nlmsg_type) {
> +		case 0:
> +			yerr(yarg->ys, YNL_ERROR_INV_RESP,
> +			     "Invalid message type in the response.");
> +			return MNL_CB_ERROR;
> +		case NLMSG_NOOP:
> +		case NLMSG_OVERRUN ... NLMSG_MIN_TYPE - 1:
I didn't know this was possible in C :D


> +			ret = MNL_CB_OK;
> +			break;
> +		case NLMSG_ERROR:
> +			ret = ynl_cb_error(nlh, yarg);
> +			break;
> +		case NLMSG_DONE:
> +			ret = ynl_cb_done(nlh, yarg);
> +			break;
> +		default:
> +			ret = cb(nlh, yarg);
> +			break;
> +		}
> +
> +		NLMSG_NEXT(nlh, rem);
> +	}
> +
> +	return ret;
>  }
>  
>  static int ynl_recv_ack(struct ynl_sock *ys, int ret)
> diff --git a/tools/net/ynl/lib/ynl.h b/tools/net/ynl/lib/ynl.h
> index ce77a6d76ce0..4849c142fce0 100644
> --- a/tools/net/ynl/lib/ynl.h
> +++ b/tools/net/ynl/lib/ynl.h
> @@ -12,6 +12,7 @@ enum ynl_error_code {
>  	YNL_ERROR_NONE = 0,
>  	__YNL_ERRNO_END = 4096,
>  	YNL_ERROR_INTERNAL,
> +	YNL_ERROR_DUMP_INTER,
>  	YNL_ERROR_EXPECT_ACK,
>  	YNL_ERROR_EXPECT_MSG,
>  	YNL_ERROR_UNEXPECT_MSG,