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.
> > > int ovpn_nl_key_get_doit(struct sk_buff *skb, struct genl_info *info)
> > > {
> > ...
> > > + slot = nla_get_u32(attrs[OVPN_A_KEYCONF_SLOT]);
> >
> >
> >
> > > int ovpn_nl_key_del_doit(struct sk_buff *skb, struct genl_info *info)
> > > {
> > ...
> > > + slot = nla_get_u8(attrs[OVPN_A_KEYCONF_SLOT]);
>
> Right. Since actual values are smaller I sometimes used the smaller macro.
Which only works on little-endian if the other side is using the full
size?
LE BE
put_u32(1) -> 01000000 00000001
get_u8 ^^ ^^
=1 =0
(and put_u8(1) vs get_u32 would result in fetching 0x1000000 in BE)
> I guess I better convert them all to u32.
SGTM
> > A few more inconsistencies:
> >
> > * OVPN_A_IFNAME is defined in the uapi but never used (I guess
> > leftover from genl link creation)
>
> Ouch, I guess you're right about it being a leftover.
>
> >
> > * OVPN_A_PEER_DEL_REASON (u32 vs u8)
> > drivers/net/ovpn/netlink-gen.c:52: [OVPN_A_PEER_DEL_REASON] = NLA_POLICY_MAX(NLA_U32, 4),
> > drivers/net/ovpn/netlink.c:1131: if (nla_put_u8(msg, OVPN_A_PEER_DEL_REASON, peer->delete_reason))
>
> Like above, probably better to convert them all to u32.
[...]
>
> u32 should be enough for packets - but yeah, going UINT is better as I don't
> need to care if it really fits 4B or not.
And that SGTM too. Thanks.
--
Sabrina
