Re: [PATCH net-next v3 05/24] ovpn: implement interface creation/destruction via netlink

Antonio Quartulli
2024-05-09T08:29:36Z

On 09/05/2024 03:09, Jakub Kicinski wrote:
> On Wed, 8 May 2024 11:49:07 +0200 Antonio Quartulli wrote:
>>>> + netdev_err(dev, "%s: cannot add ifname to reply\n", __func__);
>>>
>>> Probably not worth it, can't happen given the message size
>>
>> Personally I still prefer to check the return value of functions that
>> may fail, because somebody may break the assumption (i.e. message large
>> enough by design) without realizing that this call was relying on that.
>>
>> If you want, I could still add a comment saying that we don't expect
>> this to happen.
>
> In a few other places we put a WARN_ON_ONCE() on messages size errors.
> That way syzbot usually catches the miscalculation rather quickly.
> But no strong objections if you prefer the print.

I am fine as long as we have some check.
If WARN_ON_ONCE() helps syzbot, then I'll go with it.

>
>>>> + genlmsg_cancel(msg, hdr);
>>>> + nlmsg_free(msg);
>>>> + return -EMSGSIZE;
>>>> + }
>>>> +
>>>> + genlmsg_end(msg, hdr);
>>>> +
>>>> + return genlmsg_reply(msg, info);
>>>> }
>>>>
>>>> int ovpn_nl_del_iface_doit(struct sk_buff *skb, struct genl_info *info)
>>>> {
>>>> - return -ENOTSUPP;
>>>> + struct ovpn_struct *ovpn = info->user_ptr[0];
>>>> +
>>>> + rtnl_lock();
>>>> + ovpn_iface_destruct(ovpn);
>>>> + dev_put(ovpn->dev);
>>>> + rtnl_unlock();
>>>> +
>>>> + synchronize_net();
>>>
>>> Why? 🤔️
>>
>>
>> hmm I was under the impression that we should always call this function
>> when destroying an interface to make sure that packets that already
>> entered the network stack can be properly processed before the interface
>> is gone for good.
>>
>> Maybe this is not the right place? Any hint?
>
> The unregistration of the netdevice should take care of syncing packets
> in flight, AFAIU.

I have another call to synchronize_net() in ovpn_iface_destruct() after
calling unregister_netdevice().

Sabrina was actually questioning that call too.

First of all I now realize that we are calling it twice, but from what I
am understanding, I think we can just ditch any invocation and let core
do the right thing.

I'll remove it and do some tests.

--
Antonio Quartulli
OpenVPN Inc.