netdev - Re: [PATCH net-next v3 03/24] ovpn: add basic netlink support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4609f3bf-b79a-412a-9b9c-5fde8b7c4a99@openvpn.net>
Date: Wed, 8 May 2024 09:42:48 +0200
From: Antonio Quartulli <antonio@...nvpn.net>
To: Jakub Kicinski <kuba@...nel.org>
Cc: netdev@...r.kernel.org, Sergey Ryazanov <ryazanov.s.a@...il.com>,
 Paolo Abeni <pabeni@...hat.com>, Eric Dumazet <edumazet@...gle.com>,
 Andrew Lunn <andrew@...n.ch>, Esben Haabendal <esben@...nix.com>
Subject: Re: [PATCH net-next v3 03/24] ovpn: add basic netlink support

On 08/05/2024 02:10, Jakub Kicinski wrote:
> On Mon,  6 May 2024 03:16:16 +0200 Antonio Quartulli wrote:
>> +    name: nonce_tail_size
> 
> nit: typically we hyphenate the names in YAML and C codegen replaces
> the hyphens with underscores (and converts to uppercase)

ACK will go with hyphens.

> 
>> +         exact-len: OVPN_NONCE_TAIL_SIZE
> 
> speaking of which - is the codegen buggy or this can be nonce_tail_size?
> (or rather nonce-tail-size)

yeah, something was wrong.
I used 'nonce_tail_size' at first, but it did not get converted, 
therefore I hardcoded the final define name.

 From what you say this seems to be unexpected.
Will check what the script does.

> 
>> +      -
>> +        name: pad
>> +        type: pad
> 
> You shouldn't need this, now that we have uint.
> replace nla_put_u64_64bit() with nla_put_uint().

ACK

> Unfortunately libnl hasn't caught up so you may need to open code
> the getter a little in user space CLI.

ok, no big deal.

> 
> BTW I'd also bump the packet counters to uint.
> Doesn't cost much if they don't grow > 32b and you never know..

Ok, will do

> 
>> +        request:
>> +          attributes:
>> +            - ifname
>> +            - mode
>> +        reply:
>> +          attributes:
>> +            - ifname
> 
> The attribute lists
> 
>> +	struct net_device *dev;
>> +	int ifindex;
>> +
>> +	if (!attrs[OVPN_A_IFINDEX])
> 
> GENL_REQ_ATTR_CHECK()

ACK, I must have missed this one.

> 
>> +		return ERR_PTR(-EINVAL);
>> +
>> +	ifindex = nla_get_u32(attrs[OVPN_A_IFINDEX]);
>> +
>> +	dev = dev_get_by_index(net, ifindex);
>> +	if (!dev)
>> +		return ERR_PTR(-ENODEV);
>> +
>> +	if (!ovpn_dev_is_valid(dev))
>> +		goto err_put_dev;
>> +
>> +	return dev;
>> +
>> +err_put_dev:
>> +	dev_put(dev);
> 
> NL_SET_BAD_ATTR(info->extack, ...[OVPN_A_IFINDEX])

Oh, thanks for pointing this out.

> 
>> +	return ERR_PTR(-EINVAL);

-- 
Antonio Quartulli
OpenVPN Inc.