Re: Genetlink per cmd policies

[Jakub Kicinski <kuba@kernel.org>]

On Wed, 30 Sep 2020 22:13:07 +0200 Johannes Berg wrote:
> > +/**
> > + * struct genl_light_ops - generic netlink operations (small version)
> > + * @cmd: command identifier
> > + * @internal_flags: flags used by the family
> > + * @flags: flags
> > + * @validate: validation flags from enum genl_validate_flags
> > + * @doit: standard command callback
> > + * @dumpit: callback for dumpers
> > + *
> > + * This is a cut-down version of struct genl_ops for users who don't need
> > + * most of the ancillary infra and want to save space.
> > + */
> > +struct genl_light_ops {
> > +	int	(*doit)(struct sk_buff *skb, struct genl_info *info);
> > +	int	(*dumpit)(struct sk_buff *skb, struct netlink_callback *cb);  
> 
> Even dumpit is pretty rare (e.g. 10 out of 107 in nl80211) - maybe
> remove that even? It's a bit more juggling in nl80211 to actually use
> it, but I'm certainly happy to do that myself.

Hm. 16 / 44 for devlink if I'm counting right.

For nl80211 we'd go from:

 107 * 24 = 2471

to:

 97 * 16 + 10 * 48 = 2032

But for devlink we go from:

 44 * 24 = 1056
 
to

 16 * 16 + 28 * 48 = 1600

No strong feelings but I think the API is a little easier to grasp when
families with a global policy can use exclusively the "light" ops.

> > +static void genl_op_from_full(const struct genl_family *family,
> > +			      unsigned int i, struct genl_ops *op)
> > +{
> > +	memcpy(op, &family->ops[i], sizeof(*op));  
> 
> What's wrong with struct assignment? :)
> 
> 	*op = family->ops[i];

Code size :)

   text	   data	    bss	    dec	    hex
  22657	   3590	     64	  26311	   66c7	memcpy
  23103	   3590	     64	  26757	   6885	struct

> > +	if (!op->maxattr)
> > +		op->maxattr = family->maxattr;
> > +	if (!op->policy)
> > +		op->policy = family->policy;  
> 
> That doesn't build as is, I think? Or did you have some other patch
> below it?

Heh, right, I already have policy in ops in my tree.

I'll send a fuller RFC series by the end of the day.

> >  static int genl_validate_ops(const struct genl_family *family)
> >  {  
> [...]
> > +	n_ops = genl_get_cmd_cnt(family);
> >  	if (!n_ops)
> >  		return 0;  
> 
> Come to think of it, that check is kinda pointless, the loop won't run
> if it's 0 and then we return 0 immediately anyway... whatever :)

Good point :)

> >  	for (i = 0; i < n_ops; i++) {
> > -		if (ops[i].dumpit == NULL && ops[i].doit == NULL)
> > +		struct genl_ops op;
> > +
> > +		if (genl_get_cmd_by_index(i, family, &op))
> >  			return -EINVAL;  
> 
> Maybe WARN_ON() or something? It really ought to not be possible for
> that to fail, since you're only iterating to n_ops, so you'd have to
> have some consistency issues if that happens.
> 
> > -		for (j = i + 1; j < n_ops; j++)
> > -			if (ops[i].cmd == ops[j].cmd)
> > +		if (op.dumpit == NULL && op.doit == NULL)
> > +			return -EINVAL;
> > +		for (j = i + 1; j < n_ops; j++) {
> > +			struct genl_ops op2;
> > +
> > +			if (genl_get_cmd_by_index(j, family, &op2))
> >  				return -EINVAL;  
> 
> same here

Ack, will add it to the helper itself.

> > +		for (i = 0; i < genl_get_cmd_cnt(family); i++) {
> >  			struct nlattr *nest;
> > -			const struct genl_ops *ops = &family->ops[i];
> > -			u32 op_flags = ops->flags;
> > +			struct genl_ops op;
> > +			u32 op_flags;
> > +
> > +			if (genl_get_cmd_by_index(i, family, &op))
> > +				goto nla_put_failure;  
> 
> but actually, same here, so maybe it should just not even be able to
> return an error but WARN_ON instead and clear the op, so you have
> everything NULL in that case?
> 
> I don't really see a case where you'd have the index coming from
> userspace and would have to protect against it being bad, or something?

Yeah, this is entirely an internal error. I'll double check if cleared
out op doesn't break anything and make the helper void.