On 05/03/2025 11:11, Sabrina Dubroca wrote:
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-wi...
"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.
Ok :)
Then I'll make sure they all use nla_get/put_u32, but will still store the key_id in a u8 field.
Cheers!