Re: [libnftnl RFC 2/2] expr: Introduce struct expr_ops::attr_policy

[Phil Sutter <phil@nwl.cc>]

On Wed, Dec 13, 2023 at 09:20:35PM +0100, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > Similar to kernel's nla_policy, enable expressions to inform about
> > restrictions on attribute use. This allows the generic expression code
> > to perform sanity checks before dispatching to expression ops.
> > 
> > For now, this holds only the maximum data len which may be passed to
> > nftnl_expr_set().
> > 
> > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > ---
> >  include/expr_ops.h   |  5 +++++
> >  src/expr.c           |  6 ++++++
> >  src/expr/bitwise.c   | 11 +++++++++++
> >  src/expr/byteorder.c |  9 +++++++++
> >  src/expr/immediate.c |  9 +++++++++
> >  5 files changed, 40 insertions(+)
> > 
> > diff --git a/include/expr_ops.h b/include/expr_ops.h
> > index 51b221483552c..6c95297bfcd58 100644
> > --- a/include/expr_ops.h
> > +++ b/include/expr_ops.h
> > @@ -8,10 +8,15 @@ struct nlattr;
> >  struct nlmsghdr;
> >  struct nftnl_expr;
> >  
> > +struct attr_policy {
> > +	size_t maxlen;
> 
> I'd make this an uint32_t since that is also whats
> passed to expr_set().

ACK.

> > +		if (expr->ops->attr_policy &&
> > +		    type <= expr->ops->nftnl_max_attr &&
> > +		    expr->ops->attr_policy[type].maxlen &&
> > +		    expr->ops->attr_policy[type].maxlen < data_len)
> > +			return -1;
> > +
> 
> I'd make this more strict, is there a reason to call ops->set
> if type > ->nftnl_max_attr?

Indeed. We might even drop the default case from expressions' set
callbacks, if we assert type >= NFTNL_EXPR_BASE, too.

> Something like:
> 
> !attr_policy -> return -1;

Which means I can't procrastinate writing the policies for all 40
expressions. ;)

> type > nftnl_max_attr -> return -1:
> data_len > maxlen -> return -1.

I wanted to make this an opt-in approach, so I'd rather go with
maxlen && maxlen < data_len -> return -1.

> We could also define a minlen to disallow setting
> e.g. 1 byte of something that is internally an u32.

This at least may easily be added later, or now but with the default
minlen of 0 for most attributes.

> But I admit that this might break compatibility
> or otherwise leak internals into the api.

It's not entirely straightforward, anyway. NFTNL_EXPR_IMM_CHAIN for
instance accepts literally anything as long as it's a NUL-terminated
string. I was unsure whether libnftnl should enforce
NFT_CHAIN_MAXNAMELEN there or not.

Thanks, Phil