Re: Relax netlock to shared netlock and push it down to mrt_sysctl_mfc()
On Wed, May 17, 2023 at 01:02:58PM +0300, Vitaliy Makkoveev wrote:
> mrt_rtwalk_mfcsysctl() performs read-only access to protected data, so
> rtable_walk() could be called with shared netlock.
While I think the NET_LOCK_SHARED() is not sufficent, you can move
the NET_LOCK() into mrt_sysctl_mfc(). Only mrt_rtwalk_mfcsysctl
needs protection.
That diff would be OK bluhm@
> Index: sys/netinet/ip_input.c
> ===
> RCS file: /cvs/src/sys/netinet/ip_input.c,v
> retrieving revision 1.384
> diff -u -p -r1.384 ip_input.c
> --- sys/netinet/ip_input.c16 May 2023 19:36:00 - 1.384
> +++ sys/netinet/ip_input.c17 May 2023 09:59:16 -
> @@ -1712,10 +1712,7 @@ ip_sysctl(int *name, u_int namelen, void
> case IPCTL_MRTMFC:
> if (newp)
> return (EPERM);
> - NET_LOCK();
> - error = mrt_sysctl_mfc(oldp, oldlenp);
> - NET_UNLOCK();
> - return (error);
> + return (mrt_sysctl_mfc(oldp, oldlenp));
> case IPCTL_MRTVIF:
> if (newp)
> return (EPERM);
> Index: sys/netinet/ip_mroute.c
> ===
> RCS file: /cvs/src/sys/netinet/ip_mroute.c,v
> retrieving revision 1.138
> diff -u -p -r1.138 ip_mroute.c
> --- sys/netinet/ip_mroute.c 19 Apr 2023 20:03:51 - 1.138
> +++ sys/netinet/ip_mroute.c 17 May 2023 09:59:16 -
> @@ -479,10 +479,12 @@ mrt_sysctl_mfc(void *oldp, size_t *oldle
> msa.msa_len = *oldlenp;
> msa.msa_needed = 0;
>
> + NET_LOCK_SHARED();
> for (rtableid = 0; rtableid <= RT_TABLEID_MAX; rtableid++) {
> rtable_walk(rtableid, AF_INET, NULL, mrt_rtwalk_mfcsysctl,
> &msa);
> }
> + NET_UNLOCK_SHARED();
>
> if (msa.msa_minfos != NULL && msa.msa_needed > 0 &&
> (error = copyout(msa.msa_minfos, oldp, msa.msa_needed)) != 0) {
Re: Relax netlock to shared netlock and push it down to mrt_sysctl_mfc()
On Fri, May 26, 2023 at 06:25:46PM +0300, Vitaliy Makkoveev wrote:
> On Fri, May 26, 2023 at 05:08:06PM +0200, Alexander Bluhm wrote:
> > On Fri, May 26, 2023 at 05:29:58PM +0300, Vitaliy Makkoveev wrote:
> > > On Wed, May 17, 2023 at 01:02:58PM +0300, Vitaliy Makkoveev wrote:
> > > > mrt_rtwalk_mfcsysctl() performs read-only access to protected data, so
> > > > rtable_walk() could be called with shared netlock.
> > >
> > > Regardless on sysctl(2) unlocking backout, the netlock around
> > > mrt_sysctl_mfc() could be relaxed to shared netlock.
> >
> > IP multicast is far from MP ready. As a usual workaround I use
> > exclusive netlock or shared netlock plus kernel lock.
> >
> > Whenever I have to call something with multicast from a section
> > that has only shared netlock, I grab the kenrel lock.
> >
> > So using only NET_LOCK_SHARED() for reading multicast data does not
> > seem enough.
>
> mrt_rtwalk_mfcsysctl() does read-only access. Since the sysctl(2)
> unlocking was reverted, this will be "shared netlock plus kernel lock"
> case.
>
> > Look at the way ip_input_if() calls ip_mforward(). Maybe we should
> > start making ip_mroute.c MP safe. Unfortunately I have no test
> > environment for that.
>
> mpi@ said the kernel lock removal from uvm_swap_free() will be easy. So
> I want to try to remove it and push sysctl(2) unlocking back.
But you cannot do both. Move to shared lock in mrt_rtwalk_mfcsysctl,
and remove kernel lock from sysctl.
Write access is done with shared netlock plus kernel lock.
ip_input_if() -> ip_mforward() -> mfc_add() -> update_mfc_params() ->
mrt_mcast_del() -> rt->rt_llinfo = NULL;
So shared netlock alone is not sufficient for read access.
The popper way is to add some locking to mroute to protect itself
when running with shared netlock.
bluhm
> > > > Index: sys/netinet/ip_input.c
> > > > ===
> > > > RCS file: /cvs/src/sys/netinet/ip_input.c,v
> > > > retrieving revision 1.384
> > > > diff -u -p -r1.384 ip_input.c
> > > > --- sys/netinet/ip_input.c 16 May 2023 19:36:00 - 1.384
> > > > +++ sys/netinet/ip_input.c 17 May 2023 09:59:16 -
> > > > @@ -1712,10 +1712,7 @@ ip_sysctl(int *name, u_int namelen, void
> > > > case IPCTL_MRTMFC:
> > > > if (newp)
> > > > return (EPERM);
> > > > - NET_LOCK();
> > > > - error = mrt_sysctl_mfc(oldp, oldlenp);
> > > > - NET_UNLOCK();
> > > > - return (error);
> > > > + return (mrt_sysctl_mfc(oldp, oldlenp));
> > > > case IPCTL_MRTVIF:
> > > > if (newp)
> > > > return (EPERM);
> > > > Index: sys/netinet/ip_mroute.c
> > > > ===
> > > > RCS file: /cvs/src/sys/netinet/ip_mroute.c,v
> > > > retrieving revision 1.138
> > > > diff -u -p -r1.138 ip_mroute.c
> > > > --- sys/netinet/ip_mroute.c 19 Apr 2023 20:03:51 - 1.138
> > > > +++ sys/netinet/ip_mroute.c 17 May 2023 09:59:16 -
> > > > @@ -479,10 +479,12 @@ mrt_sysctl_mfc(void *oldp, size_t *oldle
> > > > msa.msa_len = *oldlenp;
> > > > msa.msa_needed = 0;
> > > >
> > > > + NET_LOCK_SHARED();
> > > > for (rtableid = 0; rtableid <= RT_TABLEID_MAX; rtableid++) {
> > > > rtable_walk(rtableid, AF_INET, NULL,
> > > > mrt_rtwalk_mfcsysctl,
> > > > &msa);
> > > > }
> > > > + NET_UNLOCK_SHARED();
> > > >
> > > > if (msa.msa_minfos != NULL && msa.msa_needed > 0 &&
> > > > (error = copyout(msa.msa_minfos, oldp, msa.msa_needed)) !=
> > > > 0) {
> > > >
> >
Re: Relax netlock to shared netlock and push it down to mrt_sysctl_mfc()
On Fri, May 26, 2023 at 05:08:06PM +0200, Alexander Bluhm wrote:
> On Fri, May 26, 2023 at 05:29:58PM +0300, Vitaliy Makkoveev wrote:
> > On Wed, May 17, 2023 at 01:02:58PM +0300, Vitaliy Makkoveev wrote:
> > > mrt_rtwalk_mfcsysctl() performs read-only access to protected data, so
> > > rtable_walk() could be called with shared netlock.
> > >
> >
> > Regardless on sysctl(2) unlocking backout, the netlock around
> > mrt_sysctl_mfc() could be relaxed to shared netlock.
>
> IP multicast is far from MP ready. As a usual workaround I use
> exclusive netlock or shared netlock plus kernel lock.
>
> Whenever I have to call something with multicast from a section
> that has only shared netlock, I grab the kenrel lock.
>
> So using only NET_LOCK_SHARED() for reading multicast data does not
> seem enough.
>
mrt_rtwalk_mfcsysctl() does read-only access. Since the sysctl(2)
unlocking was reverted, this will be "shared netlock plus kernel lock"
case.
> Look at the way ip_input_if() calls ip_mforward(). Maybe we should
> start making ip_mroute.c MP safe. Unfortunately I have no test
> environment for that.
>
mpi@ said the kernel lock removal from uvm_swap_free() will be easy. So
I want to try to remove it and push sysctl(2) unlocking back.
> bluhm
>
> > > Index: sys/netinet/ip_input.c
> > > ===
> > > RCS file: /cvs/src/sys/netinet/ip_input.c,v
> > > retrieving revision 1.384
> > > diff -u -p -r1.384 ip_input.c
> > > --- sys/netinet/ip_input.c16 May 2023 19:36:00 - 1.384
> > > +++ sys/netinet/ip_input.c17 May 2023 09:59:16 -
> > > @@ -1712,10 +1712,7 @@ ip_sysctl(int *name, u_int namelen, void
> > > case IPCTL_MRTMFC:
> > > if (newp)
> > > return (EPERM);
> > > - NET_LOCK();
> > > - error = mrt_sysctl_mfc(oldp, oldlenp);
> > > - NET_UNLOCK();
> > > - return (error);
> > > + return (mrt_sysctl_mfc(oldp, oldlenp));
> > > case IPCTL_MRTVIF:
> > > if (newp)
> > > return (EPERM);
> > > Index: sys/netinet/ip_mroute.c
> > > ===
> > > RCS file: /cvs/src/sys/netinet/ip_mroute.c,v
> > > retrieving revision 1.138
> > > diff -u -p -r1.138 ip_mroute.c
> > > --- sys/netinet/ip_mroute.c 19 Apr 2023 20:03:51 - 1.138
> > > +++ sys/netinet/ip_mroute.c 17 May 2023 09:59:16 -
> > > @@ -479,10 +479,12 @@ mrt_sysctl_mfc(void *oldp, size_t *oldle
> > > msa.msa_len = *oldlenp;
> > > msa.msa_needed = 0;
> > >
> > > + NET_LOCK_SHARED();
> > > for (rtableid = 0; rtableid <= RT_TABLEID_MAX; rtableid++) {
> > > rtable_walk(rtableid, AF_INET, NULL, mrt_rtwalk_mfcsysctl,
> > > &msa);
> > > }
> > > + NET_UNLOCK_SHARED();
> > >
> > > if (msa.msa_minfos != NULL && msa.msa_needed > 0 &&
> > > (error = copyout(msa.msa_minfos, oldp, msa.msa_needed)) != 0) {
> > >
>
Re: Relax netlock to shared netlock and push it down to mrt_sysctl_mfc()
On Fri, May 26, 2023 at 05:29:58PM +0300, Vitaliy Makkoveev wrote:
> On Wed, May 17, 2023 at 01:02:58PM +0300, Vitaliy Makkoveev wrote:
> > mrt_rtwalk_mfcsysctl() performs read-only access to protected data, so
> > rtable_walk() could be called with shared netlock.
> >
>
> Regardless on sysctl(2) unlocking backout, the netlock around
> mrt_sysctl_mfc() could be relaxed to shared netlock.
IP multicast is far from MP ready. As a usual workaround I use
exclusive netlock or shared netlock plus kernel lock.
Whenever I have to call something with multicast from a section
that has only shared netlock, I grab the kenrel lock.
So using only NET_LOCK_SHARED() for reading multicast data does not
seem enough.
Look at the way ip_input_if() calls ip_mforward(). Maybe we should
start making ip_mroute.c MP safe. Unfortunately I have no test
environment for that.
bluhm
> > Index: sys/netinet/ip_input.c
> > ===
> > RCS file: /cvs/src/sys/netinet/ip_input.c,v
> > retrieving revision 1.384
> > diff -u -p -r1.384 ip_input.c
> > --- sys/netinet/ip_input.c 16 May 2023 19:36:00 - 1.384
> > +++ sys/netinet/ip_input.c 17 May 2023 09:59:16 -
> > @@ -1712,10 +1712,7 @@ ip_sysctl(int *name, u_int namelen, void
> > case IPCTL_MRTMFC:
> > if (newp)
> > return (EPERM);
> > - NET_LOCK();
> > - error = mrt_sysctl_mfc(oldp, oldlenp);
> > - NET_UNLOCK();
> > - return (error);
> > + return (mrt_sysctl_mfc(oldp, oldlenp));
> > case IPCTL_MRTVIF:
> > if (newp)
> > return (EPERM);
> > Index: sys/netinet/ip_mroute.c
> > ===
> > RCS file: /cvs/src/sys/netinet/ip_mroute.c,v
> > retrieving revision 1.138
> > diff -u -p -r1.138 ip_mroute.c
> > --- sys/netinet/ip_mroute.c 19 Apr 2023 20:03:51 - 1.138
> > +++ sys/netinet/ip_mroute.c 17 May 2023 09:59:16 -
> > @@ -479,10 +479,12 @@ mrt_sysctl_mfc(void *oldp, size_t *oldle
> > msa.msa_len = *oldlenp;
> > msa.msa_needed = 0;
> >
> > + NET_LOCK_SHARED();
> > for (rtableid = 0; rtableid <= RT_TABLEID_MAX; rtableid++) {
> > rtable_walk(rtableid, AF_INET, NULL, mrt_rtwalk_mfcsysctl,
> > &msa);
> > }
> > + NET_UNLOCK_SHARED();
> >
> > if (msa.msa_minfos != NULL && msa.msa_needed > 0 &&
> > (error = copyout(msa.msa_minfos, oldp, msa.msa_needed)) != 0) {
> >
Re: Relax netlock to shared netlock and push it down to mrt_sysctl_mfc()
On Wed, May 17, 2023 at 01:02:58PM +0300, Vitaliy Makkoveev wrote:
> mrt_rtwalk_mfcsysctl() performs read-only access to protected data, so
> rtable_walk() could be called with shared netlock.
>
Regardless on sysctl(2) unlocking backout, the netlock around
mrt_sysctl_mfc() could be relaxed to shared netlock.
> Index: sys/netinet/ip_input.c
> ===
> RCS file: /cvs/src/sys/netinet/ip_input.c,v
> retrieving revision 1.384
> diff -u -p -r1.384 ip_input.c
> --- sys/netinet/ip_input.c16 May 2023 19:36:00 - 1.384
> +++ sys/netinet/ip_input.c17 May 2023 09:59:16 -
> @@ -1712,10 +1712,7 @@ ip_sysctl(int *name, u_int namelen, void
> case IPCTL_MRTMFC:
> if (newp)
> return (EPERM);
> - NET_LOCK();
> - error = mrt_sysctl_mfc(oldp, oldlenp);
> - NET_UNLOCK();
> - return (error);
> + return (mrt_sysctl_mfc(oldp, oldlenp));
> case IPCTL_MRTVIF:
> if (newp)
> return (EPERM);
> Index: sys/netinet/ip_mroute.c
> ===
> RCS file: /cvs/src/sys/netinet/ip_mroute.c,v
> retrieving revision 1.138
> diff -u -p -r1.138 ip_mroute.c
> --- sys/netinet/ip_mroute.c 19 Apr 2023 20:03:51 - 1.138
> +++ sys/netinet/ip_mroute.c 17 May 2023 09:59:16 -
> @@ -479,10 +479,12 @@ mrt_sysctl_mfc(void *oldp, size_t *oldle
> msa.msa_len = *oldlenp;
> msa.msa_needed = 0;
>
> + NET_LOCK_SHARED();
> for (rtableid = 0; rtableid <= RT_TABLEID_MAX; rtableid++) {
> rtable_walk(rtableid, AF_INET, NULL, mrt_rtwalk_mfcsysctl,
> &msa);
> }
> + NET_UNLOCK_SHARED();
>
> if (msa.msa_minfos != NULL && msa.msa_needed > 0 &&
> (error = copyout(msa.msa_minfos, oldp, msa.msa_needed)) != 0) {
>
Relax netlock to shared netlock and push it down to mrt_sysctl_mfc()
mrt_rtwalk_mfcsysctl() performs read-only access to protected data, so
rtable_walk() could be called with shared netlock.
Index: sys/netinet/ip_input.c
===
RCS file: /cvs/src/sys/netinet/ip_input.c,v
retrieving revision 1.384
diff -u -p -r1.384 ip_input.c
--- sys/netinet/ip_input.c 16 May 2023 19:36:00 - 1.384
+++ sys/netinet/ip_input.c 17 May 2023 09:59:16 -
@@ -1712,10 +1712,7 @@ ip_sysctl(int *name, u_int namelen, void
case IPCTL_MRTMFC:
if (newp)
return (EPERM);
- NET_LOCK();
- error = mrt_sysctl_mfc(oldp, oldlenp);
- NET_UNLOCK();
- return (error);
+ return (mrt_sysctl_mfc(oldp, oldlenp));
case IPCTL_MRTVIF:
if (newp)
return (EPERM);
Index: sys/netinet/ip_mroute.c
===
RCS file: /cvs/src/sys/netinet/ip_mroute.c,v
retrieving revision 1.138
diff -u -p -r1.138 ip_mroute.c
--- sys/netinet/ip_mroute.c 19 Apr 2023 20:03:51 - 1.138
+++ sys/netinet/ip_mroute.c 17 May 2023 09:59:16 -
@@ -479,10 +479,12 @@ mrt_sysctl_mfc(void *oldp, size_t *oldle
msa.msa_len = *oldlenp;
msa.msa_needed = 0;
+ NET_LOCK_SHARED();
for (rtableid = 0; rtableid <= RT_TABLEID_MAX; rtableid++) {
rtable_walk(rtableid, AF_INET, NULL, mrt_rtwalk_mfcsysctl,
&msa);
}
+ NET_UNLOCK_SHARED();
if (msa.msa_minfos != NULL && msa.msa_needed > 0 &&
(error = copyout(msa.msa_minfos, oldp, msa.msa_needed)) != 0) {
