linux-kernel - Re: [RFC PATCH] namespaces: Avoid task

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] [day] [month] [year] [list]

Message-ID: <20160226181926.GA4166@linux-uzut.site>
Date:	Fri, 26 Feb 2016 10:19:26 -0800
From:	Davidlohr Bueso <dave@...olabs.net>
To:	"Eric W. Biederman" <ebiederm@...ssion.com>
Cc:	Davidlohr Bueso <dbueso@...e.de>, viro@...iv.linux.org.uk,
	linux-kernel@...r.kernel.org,
	Linux Containers <containers@...ts.linux-foundation.org>,
	"Serge E. Hallyn" <serge@...lyn.com>
Subject: Re: [RFC PATCH] namespaces: Avoid task_lock when setting tsk->nsproxy

On Fri, 26 Feb 2016, Eric W. Biederman wrote:

>I would really appreciate it, if you are going to come up with a new
>locking primitive that you implement that locking primitive separately.

Using some rmw insn to avoid a lock is hardly implementing a new locking
primitive. This was never the purpose, and we use similar techniques throughout
the kernel to force certain paths.

>A fresh locking primitive comingled with other code is a good way to get
>something wrong, and generally to get code that is not well maintained
>because there is not a separation of concerns.
>
>Furthermore there is a world of difference between a 1+jiffy delay
>waiting for rcu_synchronize and the short hold times of task_lock.

I am aware of that, this is an optimization only based on that task_lock
is mainly unconteded, and hoped I made it clear in the changelog.

>Looking at your locking it appears to be a complete hack.  Always taking
>task_lock on read (but now you have an extra atomic op where you call
>xchg on the pointer).  Just calling compare_xchg on write if there
>are no concurrent readers.

The task on read is considered a slowpath, the extra atomic op is what I
had mentioned in the overhead, but for the fastpath we save two ops, otoh.

>For raw performance you would do better to have a separate lock, or
>potentially a specialized locking primitive that just used the low
>pointer bits.
>
>I don't know what motivates this work are you actually seeing
>performance problems with setns?

Motivation is towards other alloc_lock users, more than nsproxy itself.
I had just come across your mentioned change from using rcu and given
that there is practically 0 concurrency, though we could do better.

>I am very uncomofortable with a novel, and very awkward new locking
>primitive that does not clearly show improvements in even it's target
>workloads.
>
>Hmm.  After thinking about this a little more your set_reader_nsproxy is
>completely unsafe.  Most readers of nsproxy are from the same task.
>Changing the low bits of the pointer of from another task will cause
>those readers to segfault, and if not segfault they are reading from the
>wrong memory locations.

Ok, this part I was not sure of and was the simplest way I could think of
atomically checking for readers with a single [Rmw]. fwiw, as an alternative
to the pointer tagging I had considered doing some sorf of spin_is_locked()
check on the alloc_lock for the writer, but that has its own can of worms on
SMP anyway.

Thanks,
Davidlohr