linux-kernel - Re: [RFC][PATCH 5/7] irq_work, smp: Allow irq_work on call_single

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: <20200529133641.GM706495@hirez.programming.kicks-ass.net>
Date:   Fri, 29 May 2020 15:36:41 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Frederic Weisbecker <frederic@...nel.org>
Cc:     tglx@...utronix.de, linux-kernel@...r.kernel.org, x86@...nel.org,
        cai@....pw, mgorman@...hsingularity.net
Subject: Re: [RFC][PATCH 5/7] irq_work, smp: Allow irq_work on
 call_single_queue

On Fri, May 29, 2020 at 01:40:32AM +0200, Frederic Weisbecker wrote:
> On Tue, May 26, 2020 at 06:11:02PM +0200, Peter Zijlstra wrote:

> > +/*
> > + * structure shares layout with single_call_data_t.
> > + */
> >  struct irq_work {
> > -	atomic_t flags;
> >  	struct llist_node llnode;
> > +	atomic_t flags;
> 
> 
> We should probably have:
> 
> struct csd_node {
>        atomic_t flags;
>        struct llist_node;
> }
> 
> embed inside struct irq_work and struct __call_single_data. Relying on
> structure layout for things to work doesn't really clarify things :-)

Yes I know, but changing those structures is going to cause an aweful
lot of churn, and I didn't want to do that just now.. :-(

Also, there's more fun..

  CSD_TYPE_SYNC/ASYNC:

	struct {
		struct llist_node node;
		unsigned int flags;
		smp_call_func_t func;
		void *info;
	};

  CSD_TYPE_IRQ_WORK:

	struct {
		struct llist_node node;
		atomic_t flags;
		void (*func)(struct irq_work *);
	};

  CSD_TYPE_TTWU:

	struct {
		struct llist_node node;
		unsigned int flags;
	};

So while they all have a 'u32' sized @flags, irq_work wants it atomic.
Also, if we were to actually have the struct csd_node {}, you get a 4
byte hole when you embed it in task_struct.

This is all entirely fugly. No doubt about it.

But I failed to find a 'sane' way to express it and needed to get these
patches out because things were broken.

Maybe I can anonymous-union my way around it, dunno. I'll think about
it. I'm certainly not proud of this. But at least the BUILD_BUG_ON()s
should catch the more blatant breakage here.