linux-kernel - Re: [RFC][PATCH 4/7] inotify: Deal with nested sleeps

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, 4 Aug 2014 23:02:22 +0200
From:	Peter Zijlstra <peterz@...radead.org>
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	mingo@...nel.org, torvalds@...ux-foundation.org,
	tglx@...utronix.de, ilya.dryomov@...tank.com,
	umgwanakikbuti@...il.com, linux-kernel@...r.kernel.org,
	Eric Paris <eparis@...isplace.org>,
	John McCutchan <john@...nmccutchan.com>,
	Robert Love <rlove@...ve.org>
Subject: Re: [RFC][PATCH 4/7] inotify: Deal with nested sleeps

On Mon, Aug 04, 2014 at 09:23:58PM +0200, Oleg Nesterov wrote:
> On 08/04, Peter Zijlstra wrote:
> >
> >  	while (1) {
> > -		prepare_to_wait(&group->notification_waitq, &wait, TASK_INTERRUPTIBLE);
> > -
> >  		mutex_lock(&group->notification_mutex);
> 
> So yes, even these 2 lines look obviously buggy. Even if
> fsnotify_add_notify_event()->wake_up(&group->notification_waitq) uses
> TASK_NORMAL, so at least this can't miss an event.

There's another problem, mutex_lock() actively assumes ->state ==
TASK_RUNNING and if its not can go to sleep, possibly without ever being
woken again (because nobody knows its sleeping).

We should probably fix that too, but then its not too weird an
assumption for a blocking primitive.

> It is too later for me, but I am wondering if we can do another thing.
> Something like
> 
> 		int state;
> 
> 		prepare_to_wait(wait, TASK_INTERRUPTIBLE);
> 
> 		PUSH(&wait, state);
> 		mutex_lock();
> 		mutex_unlock();
> 		POP(&wait, state);
> 
> and, ignoring all races, lack of barriers, etc
> 
> 	#define PUSH(w, s)	s = current->state; current->state = RUNNING;
> 
> 	#define POP(w, s)	current->state = WOKEN(w) ? RUNNING : s;
> 
> Probably not... just curious.

Sure we can do a state stack, but I'm not immediately seeing the benefit
of doing so. Also I don't think we want to encourage people to do things
like this.

-rt does something like that for its spinlock->rt_mutex conversion.

In fact, you only need to push/pop around mutex_lock(), unlock will
never change state.
--
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/