Re: [PATCH v21 20/24] ovpn: implement key add/get/del/swap via netlink

[Antonio Quartulli]

On 05/03/2025 00:09, Sabrina Dubroca wrote:
> 2025-03-04, 13:11:28 +0100, Antonio Quartulli wrote:
> > On 04/03/2025 13:00, Sabrina Dubroca wrote:
> > > 2025-03-04, 01:33:50 +0100, Antonio Quartulli wrote:
> > > >    int ovpn_nl_key_new_doit(struct sk_buff *skb, struct genl_info *info)
> > > >    {
> > > ...
> > > > +	pkr.slot = nla_get_u8(attrs[OVPN_A_KEYCONF_SLOT]);
> > > > +	pkr.key.key_id = nla_get_u16(attrs[OVPN_A_KEYCONF_KEY_ID]);
> > > > +	pkr.key.cipher_alg = nla_get_u16(attrs[OVPN_A_KEYCONF_CIPHER_ALG]);
> > >
> > >
> > > [...]
> > > > +static int ovpn_nl_send_key(struct sk_buff *skb, const struct genl_info *info,
> > > > +			    u32 peer_id, enum ovpn_key_slot slot,
> > > > +			    const struct ovpn_key_config *keyconf)
> > > > +{
> > > ...
> > > > +	if (nla_put_u32(skb, OVPN_A_KEYCONF_SLOT, slot) ||
> > > > +	    nla_put_u32(skb, OVPN_A_KEYCONF_KEY_ID, keyconf->key_id) ||
> > > > +	    nla_put_u32(skb, OVPN_A_KEYCONF_CIPHER_ALG, keyconf->cipher_alg))
> > >
> > > That's a bit inconsistent. nla_put_u32 matches the generated policy,
> > > but the nla_get_u{8,16} don't (and nla_get_u16 also doesn't match "u8
> > > key_id" it's getting stored into).
> > >
> > > [also kind of curious that the policy/spec uses U32 with max values of 1/2/7]
> >
> >  From https://www.kernel.org/doc/html/next/userspace-api/netlink/specs.html#fix-width-integer-types
> >
> > "Note that types smaller than 32 bit should be avoided as using them does
> > not save any memory in Netlink messages (due to alignment)."
> >
> > Hence I went for u32 attributes, although values stored into them are much
> > smaller.
>
> Right.

What's wrong with key_id being u8 tough?

I am a bit reluctant to change all key_id fields/variables to u32, just 
because the netlink APIs prefers using u32 instead of u8.

Keeping variables/fields u8 allows to understand what values we're going 
to store internally.

And thanks to the netlink policy we know that no larger value will be 
attempted to be saved, even if the field is actually u32.


Cheers,


--
Antonio Quartulli
OpenVPN Inc.