On Wed, 4 Jun 2008 17:02:35 -0400
"J. Bruce Fields" <bfields@xxxxxxxxxxxx> wrote:
> On Wed, Jun 04, 2008 at 11:03:13AM -0400, Jeff Layton wrote:
> > diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> > index 5ac00c4..d601a77 100644
> > --- a/fs/nfsd/nfsctl.c
> > +++ b/fs/nfsd/nfsctl.c
> ...
> > @@ -566,14 +574,13 @@ static ssize_t write_versions(struct file *file, char *buf, size_t size)
> > return len;
> > }
> >
> > -static ssize_t write_ports(struct file *file, char *buf, size_t size)
> > +static ssize_t __write_ports(struct file *file, char *buf, size_t size)
> > {
> > if (size == 0) {
> > int len = 0;
> > - lock_kernel();
> > +
> > if (nfsd_serv)
> > len = svc_xprt_names(nfsd_serv, buf, 0);
> > - unlock_kernel();
>
> svc_xprt_names() has to be prepared to accept NULL as a first parameter
> (since we've got nothing here any longer to guarantee that nfsd_serv
> won't change after we've checked it). And, indeed, it does check for
> that (with its local copy, which won't change. So that's OK. But then
> could we just ditch this redundant check here? It's confusing.
>
> Oops, but: what happens if something like this races with svc_destroy,
> so svc_xprt_names() is passed a pointer to freed memory?
>
We do have a guarantee that nfsd_serv won't change after it's checked
here. The new nfsd_mutex protects it. write_ports has been renamed to
__write_ports, and write_ports has been turned into a wrapper that runs
the entire original function under the nfsd_mutex. We also have nfsd
hold the nfsd_mutex when svc_exit_thread is called, so svc_destroy
should also be called while holding it. That should serialize access
to the nfsd_serv.
I think you're correct that we can get rid of the redundant null
pointer check in __write_ports here though.
Cheers,
--
Jeff Layton <jlayton@xxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
