linux-kernel - Re: [PATCH 6/6] kmod: optional: Convert the use of xchg to a kref

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]

Message-ID: <20120328163545.GA18944@redhat.com>
Date:	Wed, 28 Mar 2012 18:35:45 +0200
From:	Oleg Nesterov <oleg@...hat.com>
To:	Boaz Harrosh <bharrosh@...asas.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>,
	linux-security-module@...r.kernel.org, Ingo Molnar <mingo@...e.hu>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Paul Turner <pjt@...gle.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	linux-fsdevel <linux-fsdevel@...r.kernel.org>,
	linux-kernel <linux-kernel@...r.kernel.org>,
	NFS list <linux-nfs@...r.kernel.org>,
	Trond Myklebust <Trond.Myklebust@...app.com>,
	"Bhamare, Sachin" <sbhamare@...asas.com>,
	David Howells <dhowells@...hat.com>,
	Eric Paris <eparis@...hat.com>,
	"Srivatsa S. Bhat" <srivatsa.bhat@...ux.vnet.ibm.com>,
	Kay Sievers <kay.sievers@...y.org>,
	James Morris <jmorris@...ei.org>,
	"Eric W. Biederman" <ebiederm@...ssion.com>,
	Greg KH <gregkh@...uxfoundation.org>,
	"Rafael J. Wysocki" <rjw@...k.pl>,
	"keyrings@...ux-nfs.org" <keyrings@...ux-nfs.org>
Subject: Re: [PATCH 6/6] kmod: optional: Convert the use of xchg to a kref

On 03/26, Boaz Harrosh wrote:
>
> if we use a kref instead of xchg to govern the lifetime
> of struct subprocess_info, the code becomes simpler and
> more generic. We can avoid some of the special cases.

Looks correct, and perhaps this makes the code more understandable.
If we use kref, we have to move the completion into subprocess_info,
but this allows to remove the "fallback to wait_for_completion()"
subtleness. Up to maintainer.

However, I agree with Peter, wait_for_completion_timeout_state()
helper from 4/6 needs more discussion.

And the previous looks patch is wrong, see my reply. The bug goes
away after this patch, but this is not good anyway. IOW, I think
you should redo 5 and 6 even if the resulting code looks correct.

Just one nit below.

> Actually I like the xchg use here. But if it will break
> some ARCHs that do not like xchg, then here is the work
> needed.

Well, xchg() has other arch-neutral users. But again, I am not
arguing with this change.

> @@ -302,7 +297,7 @@ static void __call_usermodehelper(struct work_struct *work)
>
>  	switch (wait) {
>  	case UMH_NO_WAIT:
> -		call_usermodehelper_freeinfo(sub_info);
> +		kref_put(&sub_info->kref,  call_usermodehelper_freeinfo);
>  		break;

This is minor and subjective, but UMH_NO_WAIT could use umh_complete()
too, the unnecessary complete() doesn't hurt. In this case we could do


	switch (wait) {
	case UMH_WAIT_PROC:
		if (pid > 0)
			break;
		/* FALLTHROUGH */
	case UMH_WAIT_EXEC:
		if (pid < 0)
			sub_info->retval = pid;
		/* FALLTHROUGH */
	case UMH_NO_WAIT:
		umh_complete(sub_info);
	}

Oleg.

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