linux-kernel - Re: [PATCH] lockd: convert reclaimer thread to kthread interface

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 19:19:24 -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?
> 
> 
> 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..)
> 

Forgot to answer this part...

This thread gets kicked off when the server has rebooted and we need to
reclaim our locks. There isn't a syscall on which we can return an
error to the user.

Aside from just warning the admin, I'm not sure what we can do here. We
might be able to start making all syscalls on the mount fail somehow,
but I don't think we have infrastructure for that and that may be
overkill anyway. I suppose we could also go to sleep and try to spawn the
thread again, but there's no guarantee of success there.

> 
> 
> 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.
> 
> This code needs some BKL-removal love.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


-- 
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/