linux-kernel - Re: [PATCH v2] Pre-emption control for userspace

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: <20140325111437.8e7b7c86.akpm@linux-foundation.org>
Date:	Tue, 25 Mar 2014 11:14:37 -0700
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Khalid Aziz <khalid.aziz@...cle.com>
Cc:	tglx@...utronix.de, mingo@...hat.com, hpa@...or.com,
	peterz@...radead.org, andi.kleen@...el.com, rob@...dley.net,
	viro@...iv.linux.org.uk, oleg@...hat.com,
	gnomes@...rguk.ukuu.org.uk, riel@...hat.com, snorcht@...il.com,
	dhowells@...hat.com, luto@...capital.net, daeseok.youn@...il.com,
	ebiederm@...ssion.com, linux-kernel@...r.kernel.org,
	linux-doc@...r.kernel.org
Subject: Re: [PATCH v2] Pre-emption control for userspace

On Tue, 25 Mar 2014 11:56:31 -0600 Khalid Aziz <khalid.aziz@...cle.com> wrote:

> On 03/25/2014 11:44 AM, Andrew Morton wrote:
> > So the procfs file is written in binary format and is read back in
> > ascii format.  Seems odd.
> >
> > Perhaps this should all be done as a new syscall rather than some
> > procfs thing.
> >
> 
> I didn't want to add yet another syscall which will then need to be 
> added to glibc, but I am open to doing it through a syscall if that is 
> the consensus.
> 
> >> +	struct preempt_delay {
> >> +		u32 __user *delay_req;		/* delay request flag pointer */
> >> +		unsigned char delay_granted:1;	/* currently in delay */
> >> +		unsigned char yield_penalty:1;	/* failure to yield penalty */
> >> +	} sched_preempt_delay;
> >
> > The problem with bitfields is that a write to one bitfield can corrupt
> > a concurrent write to the other one.  So it's your responsibility to
> > provide locking and/or to describe how this race is avoided.  A comment
> > here in the definition would be a suitable way of addressing this.
> >
> 
> I do not have a strong reason to use a bitfield, just trying to not use 
> any more bytes than I need to. If using a char is safer, I would rather 
> use safer code.

My point is that the locking rules should be documented, via a code comment.

Presumably that rule is "only ever modified by this task".

> >> +	if (delay_req) {
> >> +		int ret;
> >> +
> >> +		pagefault_disable();
> >> +		ret = __copy_from_user_inatomic(&delay_req_flag, delay_req,
> >> +				sizeof(u32));
> >> +		pagefault_enable();
> >
> > This all looks rather hacky and unneccesary.  Can't we somehow use
> > plain old get_user() and avoid such fuss?
> 
> get_user() takes longer and can sleep if page fault occurs. I need this 
> code to be very fast for it to be beneficial and am willing to ignore 
> page faults since page fault would imply the task has not touched 
> pre-emption delay request field and hence we can resched safely.

That's what I meant by "hacky" :)


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