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

[Xiao Liang]

On Wed, Dec 11, 2024 at 10:07 PM Antonio Quartulli <antonio@xxxxxxxxxxx> wrote:
>
> On 11/12/2024 14:53, Xiao Liang wrote:
> > On Wed, Dec 11, 2024 at 8:51 PM Antonio Quartulli <antonio@xxxxxxxxxxx> wrote:
> >>
> >> On 11/12/2024 13:35, Xiao Liang wrote:
> >>> On Wed, Dec 11, 2024 at 7:30 PM Antonio Quartulli <antonio@xxxxxxxxxxx> 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@xxxxxxxxxxx> 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.

Honestly I'm not an expert on this. Will see if I can.

>
> >
> >>>
> >>>>>
> >>>>> [...]
> >>>>>> +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.
>

That would be nice. Thanks!