linux-kernel - Re: [PATCH 2/2] ipc: semaphores: do not hold ipc lock more than necessary

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:	Fri, 1 Mar 2013 17:20:52 -0800
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Davidlohr Bueso <davidlohr.bueso@...com>
Cc:	Rik van Riel <riel@...hat.com>,
	"Vinod, Chegu" <chegu_vinod@...com>,
	"Low, Jason" <jason.low2@...com>,
	linux-tip-commits@...r.kernel.org,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	"H. Peter Anvin" <hpa@...or.com>,
	Andrew Morton <akpm@...ux-foundation.org>, aquini@...hat.com,
	Michel Lespinasse <walken@...gle.com>,
	Ingo Molnar <mingo@...nel.org>,
	Larry Woodman <lwoodman@...hat.com>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Steven Rostedt <rostedt@...dmis.org>,
	Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH 2/2] ipc: semaphores: do not hold ipc lock more than necessary

On Fri, Mar 1, 2013 at 4:16 PM, Davidlohr Bueso <davidlohr.bueso@...com> wrote:
> +static inline struct sem_array *sem_obtain_object(struct ipc_namespace *ns, int id)
> +{
> +       struct kern_ipc_perm *ipcp = ipc_obtain_object(&sem_ids(ns), id);
> +
> +       if (IS_ERR(ipcp))
> +               return (struct sem_array *)ipcp;

This should use ERR_CAST() to make it more obvious what's going on.

> +static inline struct sem_array *sem_obtain_object_check(struct ipc_namespace *ns,
> +                                                       int id)
> +{
> +       struct kern_ipc_perm *ipcp = ipc_obtain_object_check(&sem_ids(ns), id);
> +
> +       if (IS_ERR(ipcp))
> +               return (struct sem_array *)ipcp;

Same here.

> +/*
> + * Call inside the rcu read section.
> + */
> +static inline void sem_getref(struct sem_array *sma)
> +{
> +       spin_lock(&(sma)->sem_perm.lock);
> +       ipc_rcu_getref(sma);
> +       ipc_unlock(&(sma)->sem_perm);
> +}

This really makes me wonder if we shouldn't just use an atomic counter
for "refcount". But I guess that would be a separate patch.

But all the uses of refcount really look like the normal atomic ops
migth be the right thing. Especially if we no longer expect to hold
the lock most of the time.

> +               spin_lock(&sma->sem_perm.lock);

I really would almost want to make these things be "ipc_lock_object()"
rather than an open-coded spinlock like this. But that's not a big
deal.

Patch looks fine to me in general.

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