On Thu, Apr 27, 2023 at 03:22:10PM +0300, Vitaliy Makkoveev wrote:
> > On 27 Apr 2023, at 15:16, Alexander Bluhm <alexander.bl...@gmx.net> wrote:
> >
> > On Wed, Apr 26, 2023 at 11:17:37PM +0300, Vitaliy Makkoveev wrote:
> >> Route timers and route labels protected by corresponding mutexes. `ifa'
> >> uses references counting for protection. No protection required for `rt'
> >> passed to rt_mpls_clear() because only current thread owns it.
> >>
> >> ok?
> >
> > I have tested your diff and it works for me. But I don't have a
> > MPLS setup. And there is a rt_mpls_clear(rt) which your diff does
> > not show.
> >
> > if (rt->rt_llinfo != NULL)
> > free(rt->rt_llinfo);
> > rt->rt_llinfo = NULL;
> >
> > and rt->rt_flags &= ~RTF_MPLS are not mpsafe.
> >
> > What about this? Compile tested only due to lacking MPLS setup.
>
> This is not required. We hold the last reference of this dying `rt???.
You are right. OK bluhm@ for rtfree unlocking.
But I am still not convinced with rt_mpls_clear().
What about htis call path? soreceive -> pru_send -> route_send ->
route_output -> rtm_output -> rt_mpls_clear
Since solock() takes rw_enter_write(&so->so_lock) for route sockets,
we don't have the kernel lock anymore. I would feel safer with
this kenrel lock hammer for mpls. Of course rt_mpls_set() would
need something simmilar.
bluhm
> > Index: net/route.c
> > ===================================================================
> > RCS file: /data/mirror/openbsd/cvs/src/sys/net/route.c,v
> > retrieving revision 1.419
> > diff -u -p -r1.419 route.c
> > --- net/route.c 26 Apr 2023 19:54:35 -0000 1.419
> > +++ net/route.c 27 Apr 2023 12:13:01 -0000
> > @@ -1593,11 +1593,16 @@ rt_mpls_set(struct rtentry *rt, struct s
> > void
> > rt_mpls_clear(struct rtentry *rt)
> > {
> > + if (!ISSET(rt->rt_flags, RTF_MPLS))
> > + return;
> > +
> > + KERNEL_LOCK();
> > if (rt->rt_llinfo != NULL && rt->rt_flags & RTF_MPLS) {
> > free(rt->rt_llinfo, M_TEMP, sizeof(struct rt_mpls));
> > rt->rt_llinfo = NULL;
> > }
> > rt->rt_flags &= ~RTF_MPLS;
> > + KERNEL_UNLOCK();
> > }
> > #endif
> >
> >
>
