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

Jakub Kicinski
2024-05-09T01:09:48Z

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.

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