netdev - Re: [patch net-next] devlink: introduce dump selector attr and implement it for port dumps

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: <20230714084021.51fea890@kernel.org>
Date: Fri, 14 Jul 2023 08:40:21 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Jiri Pirko <jiri@...nulli.us>
Cc: netdev@...r.kernel.org, pabeni@...hat.com, davem@...emloft.net,
 edumazet@...gle.com, moshe@...dia.com
Subject: Re: [patch net-next] devlink: introduce dump selector attr and
 implement it for port dumps

On Fri, 14 Jul 2023 10:00:49 +0200 Jiri Pirko wrote:
> Fri, Jul 14, 2023 at 05:51:41AM CEST, kuba@...nel.org wrote:
> >On Thu, 13 Jul 2023 17:15:28 +0200 Jiri Pirko wrote:  
> >> +	/* If the user provided selector attribute with devlink handle, dump only
> >> +	 * objects that belong under this instance.
> >> +	 */
> >> +	if (cmd->dump_selector_nla_policy &&
> >> +	    attrs[DEVLINK_ATTR_DUMP_SELECTOR]) {
> >> +		struct nlattr *tb[DEVLINK_ATTR_MAX + 1];
> >> +
> >> +		err = nla_parse_nested(tb, DEVLINK_ATTR_MAX,
> >> +				       attrs[DEVLINK_ATTR_DUMP_SELECTOR],
> >> +				       cmd->dump_selector_nla_policy,
> >> +				       cb->extack);
> >> +		if (err)
> >> +			return err;
> >> +		if (tb[DEVLINK_ATTR_BUS_NAME] && tb[DEVLINK_ATTR_DEV_NAME]) {
> >> +			devlink = devlink_get_from_attrs_lock(sock_net(msg->sk), tb);
> >> +			if (IS_ERR(devlink))
> >> +				return PTR_ERR(devlink);
> >> +			err = cmd->dump_one(msg, devlink, cb);
> >> +			devl_unlock(devlink);
> >> +			devlink_put(devlink);
> >> +			goto out;
> >> +		}  
> >
> >This implicitly depends on the fact that cmd->dump_one() will set and
> >pay attention to state->idx. If it doesn't kernel will infinitely dump
> >the same instance. I think we should explicitly check state->idx and
> >set it to 1 after calling ->dump_one.  
> 
> Nothing changes, only instead of iterating over multiple devlinks, we
> just work with one.
> 
> So, the state->idx is in-devlink-instance index. That means, after
> iterating to next devlink instance it is reset to 0 below (state->idx = 0;).
> Here however, as we stay only within a single devlink instance,
> the reset is not needed.
> 
> Am I missing something?

The case I was thinking of is if we support filtering of dumps which
do not have sub-objects. In that case state->idx does not get touched
by the dump_one.

Looking closer, tho, there's no case of this sort today, so my concern
is premature. Also what I suggested won't really work. So ignore this
comment, sorry :)