netdev - Re: [PATCH 3/5] netlink: rework policy dump to support multiple policies

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: <7312dbde2b0bada0700afa6af417d065e45fb053.camel@sipsolutions.net>
Date:   Fri, 02 Oct 2020 22:11:14 +0200
From:   Johannes Berg <johannes@...solutions.net>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     netdev@...r.kernel.org
Subject: Re: [PATCH 3/5] netlink: rework policy dump to support multiple
 policies

On Fri, 2020-10-02 at 08:39 -0700, Jakub Kicinski wrote:
> 
> > -	ctx->state = netlink_policy_dump_start(op.policy, op.maxattr);
> > -	if (IS_ERR(ctx->state))
> > -		return PTR_ERR(ctx->state);
> > -	return 0;
> > +	return netlink_policy_dump_add_policy(&ctx->state, op.policy,
> > +					      op.maxattr);
> 
> Looks like we flip-flopped between int and pointer return between
> patches 1 and this one?

Huh, yeah, that was kinda dumb. I started going down one path and then
...

I'll probably just squash the first patch or something. Will figure
something out, thanks.

> >  }
> > +int netlink_policy_dump_get_policy_idx(struct netlink_policy_dump_state *state,
> > +				       const struct nla_policy *policy,
> > +				       unsigned int maxtype)
> > +{
> > +	unsigned int i;
> > +
> > +	if (WARN_ON(!policy || !maxtype))
> > +                return 0;
> 
> Would this warning make sense in add() (if not already there)?
> If null/0 is never added it can't match and we'd just hit the
> warning below.

It's not there, because had originally thought it should be OK to just
blindly add a policy of a family even if it has none. But that makes no
sense.

However, it's not true that it can't match, because

> > +	for (i = 0; i < state->n_alloc; i++) {

we go to n_alloc here, and don't separately track n_used, but n_alloc
grows in tens (or so), not singles.

johannes