On Tue, Apr 18, 2023 at 03:36:09PM +0200, Alexander Bluhm wrote:
> On Mon, Apr 17, 2023 at 05:53:28PM +0200, Alexander Bluhm wrote:
> > On Mon, Apr 17, 2023 at 04:32:13PM +0200, Alexander Bluhm wrote:
> > > On Mon, Apr 17, 2023 at 02:36:57AM +0300, Vitaliy Makkoveev wrote:
> > > > It seems rt_setsource() needs some attention, but sysctl_source() could
> > > > be called with shared netlock just now.
> > >
> > > I think rtable_setsource() is not MP safe. It is documented as [K]
> > > kernel lock. But that is not true and makes no sense. It should
> > > be exclusive netlock. in_pcbselsrc() calls rtable_getsource() with
> > > netlock. We should rename source to ar_source so we can grep for
> > > its users.
> >
> > Perhaps something like this. I will run it through regress.
>
> Rebased after your rename commit. Regress test did not trigger
> lock assertions. ar_source is a pointer to an ifa_addr, so net
> lock should be safe.
>
> ok?
>
ok mvs@
> bluhm
>
> Index: net/art.h
> ===================================================================
> RCS file: /cvs/src/sys/net/art.h,v
> retrieving revision 1.22
> diff -u -p -r1.22 art.h
> --- net/art.h 18 Apr 2023 10:19:16 -0000 1.22
> +++ net/art.h 18 Apr 2023 13:32:46 -0000
> @@ -41,7 +41,7 @@ struct art_root {
> uint8_t ar_nlvl; /* [I] Number of levels */
> uint8_t ar_alen; /* [I] Address length in bits */
> uint8_t ar_off; /* [I] Offset of key in bytes */
> - struct sockaddr *ar_source; /* [K] optional src addr to use
> */
> + struct sockaddr *ar_source; /* [N] use optional src addr */
> };
>
> #define ISLEAF(e) (((unsigned long)(e) & 1) == 0)
> Index: net/rtable.c
> ===================================================================
> RCS file: /cvs/src/sys/net/rtable.c,v
> retrieving revision 1.81
> diff -u -p -r1.81 rtable.c
> --- net/rtable.c 18 Apr 2023 10:19:16 -0000 1.81
> +++ net/rtable.c 18 Apr 2023 13:32:46 -0000
> @@ -376,6 +376,8 @@ rtable_setsource(unsigned int rtableid,
> {
> struct art_root *ar;
>
> + NET_ASSERT_LOCKED_EXCLUSIVE();
> +
> if ((ar = rtable_get(rtableid, af)) == NULL)
> return (EAFNOSUPPORT);
>
> @@ -388,6 +390,8 @@ struct sockaddr *
> rtable_getsource(unsigned int rtableid, int af)
> {
> struct art_root *ar;
> +
> + NET_ASSERT_LOCKED();
>
> ar = rtable_get(rtableid, af);
> if (ar == NULL)
> Index: net/rtsock.c
> ===================================================================
> RCS file: /cvs/src/sys/net/rtsock.c,v
> retrieving revision 1.362
> diff -u -p -r1.362 rtsock.c
> --- net/rtsock.c 18 Apr 2023 09:56:54 -0000 1.362
> +++ net/rtsock.c 18 Apr 2023 13:32:46 -0000
> @@ -853,8 +853,10 @@ route_output(struct mbuf *m, struct sock
> error = EINVAL;
> goto fail;
> }
> - if ((error =
> - rt_setsource(tableid, info.rti_info[RTAX_IFA])) != 0)
> + NET_LOCK();
> + error = rt_setsource(tableid, info.rti_info[RTAX_IFA]);
> + NET_UNLOCK();
> + if (error)
> goto fail;
> } else {
> error = rtm_output(rtm, &rt, &info, prio, tableid);
> @@ -862,7 +864,9 @@ route_output(struct mbuf *m, struct sock
> type = rtm->rtm_type;
> seq = rtm->rtm_seq;
> free(rtm, M_RTABLE, len);
> + NET_LOCK_SHARED();
> rtm = rtm_report(rt, type, seq, tableid);
> + NET_UNLOCK_SHARED();
> len = rtm->rtm_msglen;
> }
> }
>
