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

[Sabrina Dubroca]

2025-03-05, 02:00:21 +0100, Antonio Quartulli wrote:
> 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?

Nothing. It would make a little bit more sense to use nla_get_u16 if
key_id was u16 (even with OVPN_A_KEYCONF_KEY_ID defined as U32), or to
use nla_get_u8 for u8, but here it was just 3 different int sizes and
that triggered my "uh? what?" :)

> 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.

Sure.

> 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.

Yes.

-- 
Sabrina