Re: [PATCH net-next v14 17/22] ovpn: implement peer add/get/dump/delete via netlink

[Antonio Quartulli <antonio@openvpn.net>]

On 11/12/2024 14:53, Xiao Liang wrote:
> On Wed, Dec 11, 2024 at 8:51 PM Antonio Quartulli <antonio@openvpn.net> wrote:
>>
>> On 11/12/2024 13:35, Xiao Liang wrote:
>>> On Wed, Dec 11, 2024 at 7:30 PM Antonio Quartulli <antonio@openvpn.net> wrote:
>>>>
>>>> Hi Xiao and thanks for chiming in,
>>>>
>>>> On 11/12/2024 04:08, Xiao Liang wrote:
>>>>> On Mon, Dec 9, 2024 at 6:48 PM Antonio Quartulli <antonio@openvpn.net> wrote:
>>>>> [...]
>>>>>> +/**
>>>>>> + * ovpn_nl_peer_modify - modify the peer attributes according to the incoming msg
>>>>>> + * @peer: the peer to modify
>>>>>> + * @info: generic netlink info from the user request
>>>>>> + * @attrs: the attributes from the user request
>>>>>> + *
>>>>>> + * Return: a negative error code in case of failure, 0 on success or 1 on
>>>>>> + *        success and the VPN IPs have been modified (requires rehashing in MP
>>>>>> + *        mode)
>>>>>> + */
>>>>>> +static int ovpn_nl_peer_modify(struct ovpn_peer *peer, struct genl_info *info,
>>>>>> +                              struct nlattr **attrs)
>>>>>> +{
>>>>>> +       struct sockaddr_storage ss = {};
>>>>>> +       struct ovpn_socket *ovpn_sock;
>>>>>> +       u32 sockfd, interv, timeout;
>>>>>> +       struct socket *sock = NULL;
>>>>>> +       u8 *local_ip = NULL;
>>>>>> +       bool rehash = false;
>>>>>> +       int ret;
>>>>>> +
>>>>>> +       if (attrs[OVPN_A_PEER_SOCKET]) {
>>>>>
>>>>> Similar to link attributes in other tunnel drivers (e.g. IFLA_GRE_LINK,
>>>>> IFLA_GRE_FWMARK), user-supplied sockets could have sockopts
>>>>> (e.g. oif, fwmark, TOS). Since some of them may affect encapsulation
>>>>> and routing decision, which are supported in datapath? And do we need
>>>>> some validation here?
>>>>
>>>> Thanks for pointing this out.
>>>> At the moment ovpn doesn't expect any specific socket option.
>>>> I haven't investigated how they could be used and what effect they would
>>>> have on the packet processing.
>>>> This is something we may consider later.
>>>>
>>>> At this point, do you still think I should add a check here of some sort?
>>>>
>>>
>>> I think some sockopts are important. Especially when oif is a VRF,
>>> the destination can be totally different than using the default routing
>>> table. If we don't support them now, it would be good to deny sockets
>>> with non-default values.
>>
>> I see - openvpn in userspace doesn't set any specific oif for the
>> socket, but I understand ovpn should at least claim that those options
>> are not supported.
>>
>> I am a bit lost regarding this aspect. Do you have a pointer for me
>> where I can see how other modules are doing similar checks?
>>
> 
> The closest thing I can find is L2TP, which has some checks in
> l2tp_validate_socket(). However, it uses ip_queue_xmit() /
> inet6_csk_xmit() to send packets, where many sockopts are handled.

mhh l2tp_sk_to_tunnel() doesn't have more checks than what we already have.

> Maybe someone else can give a more suitable example. I guess we
> can start with sockopts relevant to fields in struct flowi{4,6} and encap
> headers?

Since I have little experience with sockopts in general, and this is not 
truly mission critical, how would you feel about sending a patch for 
this once ovpn has been merged?
I'd truly appreciate it.

> 
>>>
>>>>>
>>>>> [...]
>>>>>> +static int ovpn_nl_send_peer(struct sk_buff *skb, const struct genl_info *info,
>>>>>> +                            const struct ovpn_peer *peer, u32 portid, u32 seq,
>>>>>> +                            int flags)
>>>>>> +{
>>>>>> +       const struct ovpn_bind *bind;
>>>>>> +       struct nlattr *attr;
>>>>>> +       void *hdr;
>>>>>> +
>>>>>> +       hdr = genlmsg_put(skb, portid, seq, &ovpn_nl_family, flags,
>>>>>> +                         OVPN_CMD_PEER_GET);
>>>>>> +       if (!hdr)
>>>>>> +               return -ENOBUFS;
>>>>>> +
>>>>>> +       attr = nla_nest_start(skb, OVPN_A_PEER);
>>>>>> +       if (!attr)
>>>>>> +               goto err;
>>>>>> +
>>>>>> +       if (nla_put_u32(skb, OVPN_A_PEER_ID, peer->id))
>>>>>> +               goto err;
>>>>>> +
>>>>>
>>>>> I think it would be helpful to include the netns ID and supported sockopts
>>>>> of the peer socket in peer info message.
>>>>
>>>> Technically the netns is the same as where the openvpn process in
>>>> userspace is running, because it'll be it to open the socket and pass it
>>>> down to ovpn.
>>>
>>> A userspace process could open UDP sockets in one namespace
>>> and the netlink socket in another. And the ovpn link could also be
>>> moved around. At this moment, we can remember the initial netns,
>>> or perhaps link-netns, of the ovpn link, and validate if the socket
>>> is in the same one.
>>>
>>
>> You are correct, but we don't want to force sockets and link to be in
>> the same netns.
>>
>> Openvpn in userspace may have been started in the global netns, where
>> all sockets are expected to live (transport layer), but then the
>> link/device is moved - or maybe created - somewhere else (tunnel layer).
>> This is not an issue.
>>
>> Does it clarify?
> 
> If netns id is not included, then when the link has been moved,
> we can't infer which netns the socket is in from peer info message,
> thus can not figure out how packets are routed. Other tunnel drivers
> usually use IFLA_LINK_NETNSID for this. Probably have a look at
> rtnl_fill_link_netnsid()?
> 

Ok, I see what you mean.
I was assuming this was not needed, because we'd always have a running 
openvpn process and it would live in the socket netns.
But it still makes sense to report it with the peer info.

I'll add this new attribute and fill it on PEER_GET.


Thanks a lot.


> Thanks.

-- 
Antonio Quartulli
OpenVPN Inc.