This patch set continues earlier work of improving how threads and
services are managed. Specifically it drop the refcount.
The refcount is always changed under the mutex, and almost always is
exactly equal to the number of threads. Those few cases where it is
more than the number of threads can usefully be handled other ways as
see in the patches.
The first patches fixes a potential use-after-free when adding a socket
fails. This might be the UAF that Jeff mentioned recently.
The second patch which removes the use of a refcount in pool_stats
handling is more complex than I would have liked, but I think it is
worth if for the result seen in 4/5 of substantial simplification.
NeilBrown
As this function now destroys the svc_serv, this is a better name.
Signed-off-by: NeilBrown <[email protected]>
---
fs/nfsd/nfsctl.c | 4 ++--
fs/nfsd/nfsd.h | 2 +-
fs/nfsd/nfssvc.c | 8 ++++----
3 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index 86cab5281fd2..d603e672d568 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -707,7 +707,7 @@ static ssize_t __write_ports_addfd(char *buf, struct net *net, const struct cred
if (!nn->nfsd_serv->sv_nrthreads &&
list_empty(&nn->nfsd_serv->sv_permsocks))
- nfsd_last_thread(net);
+ nfsd_destroy_serv(net);
return err;
}
@@ -754,7 +754,7 @@ static ssize_t __write_ports_addxprt(char *buf, struct net *net, const struct cr
out_err:
if (!nn->nfsd_serv->sv_nrthreads &&
list_empty(&nn->nfsd_serv->sv_permsocks))
- nfsd_last_thread(net);
+ nfsd_destroy_serv(net);
return err;
}
diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h
index 9ed0e08d16c2..304e9728b929 100644
--- a/fs/nfsd/nfsd.h
+++ b/fs/nfsd/nfsd.h
@@ -148,7 +148,7 @@ int nfsd_vers(struct nfsd_net *nn, int vers, enum vers_op change);
int nfsd_minorversion(struct nfsd_net *nn, u32 minorversion, enum vers_op change);
void nfsd_reset_versions(struct nfsd_net *nn);
int nfsd_create_serv(struct net *net);
-void nfsd_last_thread(struct net *net);
+void nfsd_destroy_serv(struct net *net);
extern int nfsd_max_blksize;
diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
index 61a1d966ca48..88c2e2c94829 100644
--- a/fs/nfsd/nfssvc.c
+++ b/fs/nfsd/nfssvc.c
@@ -533,7 +533,7 @@ static struct notifier_block nfsd_inet6addr_notifier = {
/* Only used under nfsd_mutex, so this atomic may be overkill: */
static atomic_t nfsd_notifier_refcount = ATOMIC_INIT(0);
-void nfsd_last_thread(struct net *net)
+void nfsd_destroy_serv(struct net *net)
{
struct nfsd_net *nn = net_generic(net, nfsd_net_id);
struct svc_serv *serv = nn->nfsd_serv;
@@ -555,7 +555,7 @@ void nfsd_last_thread(struct net *net)
/*
* write_ports can create the server without actually starting
* any threads--if we get shut down before any threads are
- * started, then nfsd_last_thread will be run before any of this
+ * started, then nfsd_destroy_serv will be run before any of this
* other initialization has been done except the rpcb information.
*/
svc_rpcb_cleanup(serv, net);
@@ -641,7 +641,7 @@ void nfsd_shutdown_threads(struct net *net)
/* Kill outstanding nfsd threads */
svc_set_num_threads(serv, NULL, 0);
- nfsd_last_thread(net);
+ nfsd_destroy_serv(net);
mutex_unlock(&nfsd_mutex);
}
@@ -802,7 +802,7 @@ nfsd_svc(int nrservs, struct net *net, const struct cred *cred)
error = serv->sv_nrthreads;
out_put:
if (serv->sv_nrthreads == 0)
- nfsd_last_thread(net);
+ nfsd_destroy_serv(net);
out:
mutex_unlock(&nfsd_mutex);
return error;
--
2.42.0
On Mon, 2023-10-30 at 12:08 +1100, NeilBrown wrote:
> As this function now destroys the svc_serv, this is a better name.
>
> Signed-off-by: NeilBrown <[email protected]>
> ---
> fs/nfsd/nfsctl.c | 4 ++--
> fs/nfsd/nfsd.h | 2 +-
> fs/nfsd/nfssvc.c | 8 ++++----
> 3 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> index 86cab5281fd2..d603e672d568 100644
> --- a/fs/nfsd/nfsctl.c
> +++ b/fs/nfsd/nfsctl.c
> @@ -707,7 +707,7 @@ static ssize_t __write_ports_addfd(char *buf, struct net *net, const struct cred
>
> if (!nn->nfsd_serv->sv_nrthreads &&
> list_empty(&nn->nfsd_serv->sv_permsocks))
> - nfsd_last_thread(net);
> + nfsd_destroy_serv(net);
>
> return err;
> }
> @@ -754,7 +754,7 @@ static ssize_t __write_ports_addxprt(char *buf, struct net *net, const struct cr
> out_err:
> if (!nn->nfsd_serv->sv_nrthreads &&
> list_empty(&nn->nfsd_serv->sv_permsocks))
> - nfsd_last_thread(net);
> + nfsd_destroy_serv(net);
>
> return err;
> }
> diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h
> index 9ed0e08d16c2..304e9728b929 100644
> --- a/fs/nfsd/nfsd.h
> +++ b/fs/nfsd/nfsd.h
> @@ -148,7 +148,7 @@ int nfsd_vers(struct nfsd_net *nn, int vers, enum vers_op change);
> int nfsd_minorversion(struct nfsd_net *nn, u32 minorversion, enum vers_op change);
> void nfsd_reset_versions(struct nfsd_net *nn);
> int nfsd_create_serv(struct net *net);
> -void nfsd_last_thread(struct net *net);
> +void nfsd_destroy_serv(struct net *net);
>
> extern int nfsd_max_blksize;
>
> diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
> index 61a1d966ca48..88c2e2c94829 100644
> --- a/fs/nfsd/nfssvc.c
> +++ b/fs/nfsd/nfssvc.c
> @@ -533,7 +533,7 @@ static struct notifier_block nfsd_inet6addr_notifier = {
> /* Only used under nfsd_mutex, so this atomic may be overkill: */
> static atomic_t nfsd_notifier_refcount = ATOMIC_INIT(0);
>
> -void nfsd_last_thread(struct net *net)
> +void nfsd_destroy_serv(struct net *net)
> {
> struct nfsd_net *nn = net_generic(net, nfsd_net_id);
> struct svc_serv *serv = nn->nfsd_serv;
> @@ -555,7 +555,7 @@ void nfsd_last_thread(struct net *net)
> /*
> * write_ports can create the server without actually starting
> * any threads--if we get shut down before any threads are
> - * started, then nfsd_last_thread will be run before any of this
> + * started, then nfsd_destroy_serv will be run before any of this
> * other initialization has been done except the rpcb information.
> */
> svc_rpcb_cleanup(serv, net);
> @@ -641,7 +641,7 @@ void nfsd_shutdown_threads(struct net *net)
>
> /* Kill outstanding nfsd threads */
> svc_set_num_threads(serv, NULL, 0);
> - nfsd_last_thread(net);
> + nfsd_destroy_serv(net);
> mutex_unlock(&nfsd_mutex);
> }
>
> @@ -802,7 +802,7 @@ nfsd_svc(int nrservs, struct net *net, const struct cred *cred)
> error = serv->sv_nrthreads;
> out_put:
> if (serv->sv_nrthreads == 0)
> - nfsd_last_thread(net);
> + nfsd_destroy_serv(net);
> out:
> mutex_unlock(&nfsd_mutex);
> return error;
LGTM
Reviewed-by: Jeff Layton <[email protected]>
> On Oct 29, 2023, at 6:08 PM, NeilBrown <[email protected]> wrote:
>
> This patch set continues earlier work of improving how threads and
> services are managed. Specifically it drop the refcount.
>
> The refcount is always changed under the mutex, and almost always is
> exactly equal to the number of threads. Those few cases where it is
> more than the number of threads can usefully be handled other ways as
> see in the patches.
>
> The first patches fixes a potential use-after-free when adding a socket
> fails. This might be the UAF that Jeff mentioned recently.
>
> The second patch which removes the use of a refcount in pool_stats
> handling is more complex than I would have liked, but I think it is
> worth if for the result seen in 4/5 of substantial simplification.
So I need a v2 of this series, then...
- Move 4/5 to the beginning of the series (patch 1 or 2, doesn't matter)
- Add R-b, Fixes, and Cc: stable tags to those two patches
- Address Jeff's other comments
AFAICT the 0-day bot reports were for the admin-revoked state series,
not this one.
--
Chuck Lever
On Wed, 01 Nov 2023, Chuck Lever III wrote:
>
> > On Oct 29, 2023, at 6:08 PM, NeilBrown <[email protected]> wrote:
> >
> > This patch set continues earlier work of improving how threads and
> > services are managed. Specifically it drop the refcount.
> >
> > The refcount is always changed under the mutex, and almost always is
> > exactly equal to the number of threads. Those few cases where it is
> > more than the number of threads can usefully be handled other ways as
> > see in the patches.
> >
> > The first patches fixes a potential use-after-free when adding a socket
> > fails. This might be the UAF that Jeff mentioned recently.
> >
> > The second patch which removes the use of a refcount in pool_stats
> > handling is more complex than I would have liked, but I think it is
> > worth if for the result seen in 4/5 of substantial simplification.
>
> So I need a v2 of this series, then...
>
> - Move 4/5 to the beginning of the series (patch 1 or 2, doesn't matter)
I don't understand.... 2 and 3 are necessary prerequisites for 4. The
remove places where the refcount is used.
>
> - Add R-b, Fixes, and Cc: stable tags to those two patches
Will do
>
> - Address Jeff's other comments
Yep.
>
> AFAICT the 0-day bot reports were for the admin-revoked state series,
> not this one.
Agreed.
NeilBrown
>
>
> --
> Chuck Lever
>
>
>
> On Oct 31, 2023, at 1:40 PM, NeilBrown <[email protected]> wrote:
>
> On Wed, 01 Nov 2023, Chuck Lever III wrote:
>>
>>> On Oct 29, 2023, at 6:08 PM, NeilBrown <[email protected]> wrote:
>>>
>>> This patch set continues earlier work of improving how threads and
>>> services are managed. Specifically it drop the refcount.
>>>
>>> The refcount is always changed under the mutex, and almost always is
>>> exactly equal to the number of threads. Those few cases where it is
>>> more than the number of threads can usefully be handled other ways as
>>> see in the patches.
>>>
>>> The first patches fixes a potential use-after-free when adding a socket
>>> fails. This might be the UAF that Jeff mentioned recently.
>>>
>>> The second patch which removes the use of a refcount in pool_stats
>>> handling is more complex than I would have liked, but I think it is
>>> worth if for the result seen in 4/5 of substantial simplification.
>>
>> So I need a v2 of this series, then...
>>
>> - Move 4/5 to the beginning of the series (patch 1 or 2, doesn't matter)
>
> I don't understand.... 2 and 3 are necessary prerequisites for 4. The
> remove places where the refcount is used.
Hrm. that's what I was afraid of.
Isn't there a fix in 4/5 that we want applied to stable kernels,
or did I misunderstand the email exchange between you and Jeff?
--
Chuck Lever
On Wed, 01 Nov 2023, Chuck Lever III wrote:
>
> > On Oct 31, 2023, at 1:40 PM, NeilBrown <[email protected]> wrote:
> >
> > On Wed, 01 Nov 2023, Chuck Lever III wrote:
> >>
> >>> On Oct 29, 2023, at 6:08 PM, NeilBrown <[email protected]> wrote:
> >>>
> >>> This patch set continues earlier work of improving how threads and
> >>> services are managed. Specifically it drop the refcount.
> >>>
> >>> The refcount is always changed under the mutex, and almost always is
> >>> exactly equal to the number of threads. Those few cases where it is
> >>> more than the number of threads can usefully be handled other ways as
> >>> see in the patches.
> >>>
> >>> The first patches fixes a potential use-after-free when adding a socket
> >>> fails. This might be the UAF that Jeff mentioned recently.
> >>>
> >>> The second patch which removes the use of a refcount in pool_stats
> >>> handling is more complex than I would have liked, but I think it is
> >>> worth if for the result seen in 4/5 of substantial simplification.
> >>
> >> So I need a v2 of this series, then...
> >>
> >> - Move 4/5 to the beginning of the series (patch 1 or 2, doesn't matter)
> >
> > I don't understand.... 2 and 3 are necessary prerequisites for 4. The
> > remove places where the refcount is used.
>
> Hrm. that's what I was afraid of.
>
> Isn't there a fix in 4/5 that we want applied to stable kernels,
> or did I misunderstand the email exchange between you and Jeff?
>
That is
Commit bf32075256e9 ("NFSD: simplify error paths in nfsd_svc()")
which is already in nfsd-next.
NeilBrown
>
> --
> Chuck Lever
>
>
>
> On Oct 31, 2023, at 1:46 PM, NeilBrown <[email protected]> wrote:
>
> On Wed, 01 Nov 2023, Chuck Lever III wrote:
>>
>>> On Oct 31, 2023, at 1:40 PM, NeilBrown <[email protected]> wrote:
>>>
>>> On Wed, 01 Nov 2023, Chuck Lever III wrote:
>>>>
>>>>> On Oct 29, 2023, at 6:08 PM, NeilBrown <[email protected]> wrote:
>>>>>
>>>>> This patch set continues earlier work of improving how threads and
>>>>> services are managed. Specifically it drop the refcount.
>>>>>
>>>>> The refcount is always changed under the mutex, and almost always is
>>>>> exactly equal to the number of threads. Those few cases where it is
>>>>> more than the number of threads can usefully be handled other ways as
>>>>> see in the patches.
>>>>>
>>>>> The first patches fixes a potential use-after-free when adding a socket
>>>>> fails. This might be the UAF that Jeff mentioned recently.
>>>>>
>>>>> The second patch which removes the use of a refcount in pool_stats
>>>>> handling is more complex than I would have liked, but I think it is
>>>>> worth if for the result seen in 4/5 of substantial simplification.
>>>>
>>>> So I need a v2 of this series, then...
>>>>
>>>> - Move 4/5 to the beginning of the series (patch 1 or 2, doesn't matter)
>>>
>>> I don't understand.... 2 and 3 are necessary prerequisites for 4. The
>>> remove places where the refcount is used.
>>
>> Hrm. that's what I was afraid of.
>>
>> Isn't there a fix in 4/5 that we want applied to stable kernels,
>> or did I misunderstand the email exchange between you and Jeff?
>>
> That is
> Commit bf32075256e9 ("NFSD: simplify error paths in nfsd_svc()")
> which is already in nfsd-next.
OK. I'm still confused, but let's just keep the patch ordering
the way it was in v1, then. We'll work out any missing fixes
as they arise.
--
Chuck Lever