lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Mon, 3 Nov 2008 17:28:50 -0500
From:	Jeff Layton <jlayton@...hat.com>
To:	Andrew Morton <akpm@...ux-foundation.org>
Cc:	trond.myklebust@....uio.no, linux-nfs@...r.kernel.org,
	linux-kernel@...r.kernel.org, hch@...radead.org
Subject: Re: [PATCH] lockd: convert reclaimer thread to kthread interface

On Mon, 3 Nov 2008 13:12:15 -0800
Andrew Morton <akpm@...ux-foundation.org> wrote:

> On Wed, 29 Oct 2008 07:15:45 -0400
> Jeff Layton <jlayton@...hat.com> wrote:
> 
> > My understanding is that there is a push to turn the kernel_thread
> > interface into a non-exported symbol and move all kernel threads to use
> > the kthread API. This patch changes lockd to use kthread_run to spawn
> > the reclaimer thread.
> > 
> > I've made the assumption here that the extra module references taken
> > when we spawn this thread are unnecessary and removed them. I've also
> > added a KERN_ERR printk that pops if the thread can't be spawned to warn
> > the admin that the locks won't be reclaimed.
> > 
> > I consider this patch 2.6.29 material.
> > 
> > Signed-off-by: Jeff Layton <jlayton@...hat.com>
> > ---
> >  fs/lockd/clntlock.c |   14 +++++++++-----
> >  1 files changed, 9 insertions(+), 5 deletions(-)
> > 
> > diff --git a/fs/lockd/clntlock.c b/fs/lockd/clntlock.c
> > index 8307dd6..fcc2378 100644
> > --- a/fs/lockd/clntlock.c
> > +++ b/fs/lockd/clntlock.c
> > @@ -14,6 +14,7 @@
> >  #include <linux/sunrpc/svc.h>
> >  #include <linux/lockd/lockd.h>
> >  #include <linux/smp_lock.h>
> > +#include <linux/kthread.h>
> >  
> >  #define NLMDBG_FACILITY		NLMDBG_CLIENT
> >  
> > @@ -191,11 +192,15 @@ __be32 nlmclnt_grant(const struct sockaddr *addr, const struct nlm_lock *lock)
> >  void
> >  nlmclnt_recovery(struct nlm_host *host)
> >  {
> > +	struct task_struct *task;
> > +
> >  	if (!host->h_reclaiming++) {
> >  		nlm_get_host(host);
> > -		__module_get(THIS_MODULE);
> > -		if (kernel_thread(reclaimer, host, CLONE_FS | CLONE_FILES) < 0)
> > -			module_put(THIS_MODULE);
> > +		task = kthread_run(reclaimer, host, "%s-reclaim", host->h_name);
> > +		if (IS_ERR(task))
> > +			printk(KERN_ERR "lockd: unable to spawn reclaimer "
> > +				"thread. Locks for %s won't be reclaimed! "
> > +				"(%ld)\n", host->h_name, PTR_ERR(task));
> >  	}
> >  }
> >  
> > @@ -207,7 +212,6 @@ reclaimer(void *ptr)
> >  	struct file_lock *fl, *next;
> >  	u32 nsmstate;
> >  
> > -	daemonize("%s-reclaim", host->h_name);
> >  	allow_signal(SIGKILL);
> >  
> >  	down_write(&host->h_rwsem);
> > @@ -261,5 +265,5 @@ restart:
> >  	nlm_release_host(host);
> >  	lockd_down();
> >  	unlock_kernel();
> > -	module_put_and_exit(0);
> > +	return 0;
> >  }
> 
> Looks OK to me.  I assume the SIGKILL handling has been carefully tested?
> 

Not by me, though I don't think this patch will make that any better or
worse. It should just change how the thread is spawned. My testing
mostly consisted of making sure that we could reclaim locks after this
was applied.

> 
> Is it correct to emit a warning and keep going if the thread didn't
> start?  Or would it be safer&saner to fail the whole mount (or whatever
> syscall we're doing here..)
> 
> 
> 
> I see this:
> 
> 	/* Why are we leaking memory here? --okir */
> 	if (signalled())
> 		continue;
> 
> is that still true?  It seems unlikely that what appears to be a pretty
> gross leak has been around for so long.
> 

Well, just before that we do this:

    list_del_init(&fl->fl_u.nfs_fl.list);

...and a little while after, we do this:

    list_add_tail(&fl->fl_u.nfs_fl.list, &host->h_granted);

...so I assume the fact that the fl doesn't end up back on a list if
we're signalled is the problem. If so, then yes, it does look like we're
still leaking memory there.

I've never heard of anyone needing to signal the reclaimer thread, so
maybe we should just make it ignore all signals?

> This code needs some BKL-removal love.

Yep. All of lockd does, though IIRC that's held up by dependencies on
the BKL in generic VFS locking code. Pulling the BKL out of lockd is
probably going to be painful since it's almost surely hiding some
races.

-- 
Jeff Layton <jlayton@...hat.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists