netdev - Re: [PATCH ethtool-next v2 5/6] netlink: use policy dumping to check if stats flag is supported

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] [day] [month] [year] [list]

Message-ID: <20201012154455.tq65ttu6mrpoocyj@lion.mk-sys.cz>
Date:   Mon, 12 Oct 2020 17:44:55 +0200
From:   Michal Kubecek <mkubecek@...e.cz>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     netdev@...r.kernel.org, kernel-team@...com
Subject: Re: [PATCH ethtool-next v2 5/6] netlink: use policy dumping to check
 if stats flag is supported

On Tue, Oct 06, 2020 at 08:04:24AM -0700, Jakub Kicinski wrote:
> Older kernels don't support statistics, to avoid retries
> make use of netlink policy dumps to figure out which
> flags kernel actually supports.
> 
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>
> ---
[...]
> +static int family_policy_cb(const struct nlmsghdr *nlhdr, void *data)
> +{
> +	const struct nlattr *tba[NL_POLICY_TYPE_ATTR_MAX + 1] = {};
> +	DECLARE_ATTR_TB_INFO(tba);
> +	const struct nlattr *tb[CTRL_ATTR_MAX + 1] = {};
> +	DECLARE_ATTR_TB_INFO(tb);
> +	const struct nlattr *policy_attr, *attr_attr, *attr;
> +	struct ethtool_op_policy_query_ctx *ctx = data;

I would rather use a different name for this variable as "ctx" is used
for struct cmd_context everywhere else.

> +	unsigned int attr_idx, policy_idx;
> +	int ret;

[...]
> +static int get_flags_policy(struct nl_context *nlctx, struct nl_socket *nlsk,
> +			    unsigned int nlcmd, unsigned int hdrattr)
> +{
> +	struct nl_msg_buff *msgbuff = &nlsk->msgbuff;
> +	struct ethtool_op_policy_query_ctx ctx;

Same here.

> +	int ret;
> +
> +	memset(&ctx, 0, sizeof(ctx));
> +	ctx.nlctx = nlctx;
> +	ctx.op = nlcmd;
> +	ctx.op_hdr_attr = hdrattr;
> +
> +	ret = __msg_init(msgbuff, GENL_ID_CTRL, CTRL_CMD_GETPOLICY,
> +			 NLM_F_REQUEST | NLM_F_ACK | NLM_F_DUMP, 1);
> +	if (ret < 0)
> +		return ret;
> +	ret = -EMSGSIZE;
> +	if (ethnla_put_u16(msgbuff, CTRL_ATTR_FAMILY_ID, nlctx->ethnl_fam))
> +		return ret;
> +	if (ethnla_put_u32(msgbuff, CTRL_ATTR_OP, nlcmd))
> +		return ret;
> +
> +	nlsock_sendmsg(nlsk, NULL);
> +	nlsock_process_reply(nlsk, family_policy_cb, &ctx);
> +
> +	ret = ctx.flag_mask;
> +	return ret;
> +}

The return value is assigned either a negative error code or a u32 flag
mask. If we run this code on a kernel supporting 32 flags, flag in bit
31 would collide with sign and successfully retrieved flag mask would be
interpreted as an error by caller.

Other than this, the series looks good to me.

Michal

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)