linux-kernel - Re: [PATCH 5/6] NLM: Convert lockd to use kthreads

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:	Wed, 9 Jan 2008 13:08:11 -0500
From:	Jeff Layton <jlayton@...hat.com>
To:	Christoph Hellwig <hch@...radead.org>
Cc:	akpm@...ux-foundation.org, neilb@...e.de,
	linux-nfs@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 5/6] NLM: Convert lockd to use kthreads

On Wed, 9 Jan 2008 17:45:06 +0000
Christoph Hellwig <hch@...radead.org> wrote:

> On Tue, Jan 08, 2008 at 02:33:17PM -0500, Jeff Layton wrote:
> > -	struct svc_serv *	serv;
> > -	int			error = 0;
> > +	struct svc_serv *serv;
> > +	struct svc_rqst *rqstp;
> > +	int		error = 0;
> >  
> >  	mutex_lock(&nlmsvc_mutex);
> >  	/*
> >  	 * Check whether we're already up and running.
> >  	 */
> > -	if (nlmsvc_pid) {
> > +	if (nlmsvc_task) {
> >  		if (proto)
> >  			error = make_socks(nlmsvc_serv, proto);
> 
> While equivalent I think it would be clener to check for nlmsvc_serv
> above as that'swhat we're passing to make_socks.  But I think the
> whole of lockd_up could use a little makeover, but that's for later.
> 

Probably so. If I respin, I'll plan to fix that too.

> >  void
> >  lockd_down(void)
> >  {
> >  	mutex_lock(&nlmsvc_mutex);
> >  	if (nlmsvc_users) {
> >  		if (--nlmsvc_users)
> >  			goto out;
> > +	} else {
> > +		printk(KERN_ERR "lockd_down: no users! task=%p\n",
> > +			nlmsvc_task);
> > +		BUG();
> >  	}
> > +	if (!nlmsvc_task) {
> > +		printk(KERN_ERR "lockd_down: no lockd running.\n");
> > +		BUG();
> >  	}
> > +	kthread_stop(nlmsvc_task);
> 
> I think all this user/foo checking here should be BUG_ONs as it's
> quite fatal errors.
> 
> e.g.
> 
> void
> lockd_down(void)
> {
>  	mutex_lock(&nlmsvc_mutex);
> 
> 	BUG_ON(!nlmsvc_task);
> 	BUG_ON(!nlmsvc_users);
> 
> 	if (!--nlmsvc_users)
> 		kthread_stop(nlmsvc_task);
> 	mutex_unlock(&nlmsvc_mutex);
> }
> 
> 
> same applies for similar checks in lockd_up aswell.
> 	

With this patch the lockd_down checks should now be BUGs. I decided
not to do that in lockd_up. If there's an error within the main
lockd loop, it can exit without being requested to do so. If someone
then calls lockd_up then the counts will be off and the check will fire.

It seems like if we're going to make the check in lockd_up be a BUG,
then we should also BUG rather than letting lockd exit prematurely.

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