Re: [PATCH net-next v3 07/24] ovpn: introduce the ovpn_peer object

[Sabrina Dubroca <sd@queasysnail.net>]

2024-05-09, 15:44:26 +0200, Antonio Quartulli wrote:
> On 09/05/2024 15:04, Sabrina Dubroca wrote:
> > > > > +void ovpn_peer_release(struct ovpn_peer *peer)
> > > > > +{
> > > > > +	call_rcu(&peer->rcu, ovpn_peer_release_rcu);
> > > > > +}
> > > > > +
> > > > > +/**
> > > > > + * ovpn_peer_delete_work - work scheduled to release peer in process context
> > > > > + * @work: the work object
> > > > > + */
> > > > > +static void ovpn_peer_delete_work(struct work_struct *work)
> > > > > +{
> > > > > +	struct ovpn_peer *peer = container_of(work, struct ovpn_peer,
> > > > > +					      delete_work);
> > > > > +	ovpn_peer_release(peer);
> > > > 
> > > > Does call_rcu really need to run in process context?
> > > 
> > > Reason for switching to process context is that we have to invoke
> > > ovpn_nl_notify_del_peer (that sends a netlink event to userspace) and the
> > > latter requires a reference to the peer.
> > 
> > I'm confused. When you say "requires a reference to the peer", do you
> > mean accessing fields of the peer object? I don't see why this
> > requires ovpn_nl_notify_del_peer to to run from process context.
> 
> ovpn_nl_notify_del_peer sends a netlink message to userspace and I was under
> the impression that it may block/sleep, no?
> For this reason I assumed it must be executed in process context.

With s/GFP_KERNEL/GFP_ATOMIC/, it should be ok to run from whatever
context. Firing up a workqueue just to send a 100B netlink message
seems a bit overkill.



> This said, I have a question regarding DEBUG_NET_WARN_ON_ONCE: it prints
> something only if CONFIG_DEBUG_NET is enabled.
> Is this the case on standard desktop/server distribution? Otherwise how are
> we going to get reports from users?

That's pretty much why I'm suggesting to use it. For those things that
should really never happen, I think letting developers find them
during testing (or syzbot when it gets to your driver) is enough. I'm
not convinced getting a stack trace from a user without any ability to
reproduce is that useful.

But if you or someone else really want some WARN_ONs, I can live with
that.

> > > > And if this happens during interface deletion, aren't we leaking the
> > > > peer memory here?
> > > 
> > > at interface deletion we call
> > > 
> > > ovpn_iface_destruct -> ovpn_peer_release_p2p ->
> > > ovpn_peer_del_p2p(ovpn->peer)
> > > 
> > > so at the last step we just ask to remove the very same peer that is
> > > curently stored, which should just never fail.
> > 
> > But that's not what the test checks for. If ovpn->peer->ovpn != ovpn,
> > the test in ovpn_peer_del_p2p will fail. That's "objects getting out
> > of sync" in my previous email. The peer has a bogus back reference to
> > its ovpn parent, but it's ovpn->peer nevertheless.
> > 
> 
> Oh thanks for explaining that.
> 
> Ok, my assumption is that "ovpn->peer->ovpn != ovpn" can never be true.
> 
> Peers are created within the context of one ovpn object and are never
> exposed to other ovpns.
> 
> I hope it makes sense.

Ok, so this would indicate that something has gone badly wrong. Is it
really worth checking for that (or maybe just during development)?

-- 
Sabrina