linux-kernel - Re: [PATCH 3/3] fork, vhost: Use CLONE_THREAD to fix freezer/ps regression

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: <20230601074315.GA13133@redhat.com>
Date:   Thu, 1 Jun 2023 09:43:16 +0200
From:   Oleg Nesterov <oleg@...hat.com>
To:     Jason Wang <jasowang@...hat.com>
Cc:     Mike Christie <michael.christie@...cle.com>, linux@...mhuis.info,
        nicolas.dichtel@...nd.com, axboe@...nel.dk, ebiederm@...ssion.com,
        torvalds@...ux-foundation.org, linux-kernel@...r.kernel.org,
        virtualization@...ts.linux-foundation.org, mst@...hat.com,
        sgarzare@...hat.com, stefanha@...hat.com, brauner@...nel.org
Subject: Re: [PATCH 3/3] fork, vhost: Use CLONE_THREAD to fix freezer/ps
 regression

On 06/01, Jason Wang wrote:
>
> On Wed, May 31, 2023 at 5:14 PM Oleg Nesterov <oleg@...hat.com> wrote:
> >
> > > > I don't understand you. OK, to simplify, suppose we have 2 global vars
> > > >
> > > >         void *PTR = something_non_null;
> > > >         unsigned long FLAGS = -1ul;
> > > >
> > > > Now I think this code
> > > >
> > > >         CPU_0                           CPU_1
> > > >
> > > >         void *ptr = PTR;                if (!test_and_set_bit(0, FLAGS))
> > > >         clear_bit(0, FLAGS);                    PTR = NULL;
> > > >         BUG_ON(!ptr);
> > > >
> > > > is racy and can hit the BUG_ON(!ptr).
> > >
> > > This seems different to the above case?
> >
> > not sure,
> >
> > > And you can hit BUG_ON with
> > > the following execution sequence:
> > >
> > > [cpu 0] clear_bit(0, FLAGS);
> > > [cpu 1] if (!test_and_set_bit(0, FLAGS))
> > > [cpu 1] PTR = NULL;
> > > [cpu 0] BUG_ON(!ptr)
> >
> > I don't understand this part... yes, we can hit this BUG_ON() without mb in
> > between, this is what I tried to say.
>
> I may miss something,

Or me... note that CPU_0 loads the global "PTR" into the local "ptr" before clear_bit.
Since you have mentioned the program order: yes this lacks READ_ONCE() or barrier(),
but the same is true for the code in vhost_worker(). So I still don't understand.

> but the above is the sequence that is executed
> by the processor (for each CPU, it's just the program order). So where
> do you expect to place an mb can help?

before clear_bit:

	CPU_0

	void *ptr = PTR;
	mb();			// implies compiler barrier as well
	clear_bit(0, FLAGS);
	BUG_ON(!ptr);

just in case... mb() in the code above is only for illustration, we can use
smp_mb__before_atomic() + clear_bit(). Or just clear_bit_unlock(), iiuc the
one-way barrier is fine in this case.


> > > In vhost code, there's a condition before the clear_bit() which sits
> > > inside llist_for_each_entry_safe():
> > >
> > > #define llist_for_each_entry_safe(pos, n, node, member)                        \
> > >         for (pos = llist_entry((node), typeof(*pos), member);                  \
> > >              member_address_is_nonnull(pos, member) &&                         \
> > >                 (n = llist_entry(pos->member.next, typeof(*n), member), true); \
> > >              pos = n)
> > >
> > > The clear_bit() is a store which is not speculated, so there's a
> > > control dependency, the store can't be executed until the condition
> > > expression is evaluated which requires pos->member.next
> > > (work->node.next) to be loaded.
> >
> > But llist_for_each_entry_safe() doesn't check "n", I mean, it is not that we have
> > something like
> >
> >         n = llist_entry(...);
> >         if (n)
> >                 clear_bit(...);
> >
> > so I do not see how can we rely on the load-store control dependency.
>
> Just to make sure we are on the same page, the condition expression is
>
> member_address_is_nonnull(pos, member) && (n =
> llist_entry(pos->member.next, typeof(*n), member), true)
>
> So it's something like:
>
> if (work->node && (work_next = work->node->next, true))
>     clear_bit(&work->flags);
>
> So two loads from both work->node and work->node->next, and there's a
> store which is clear_bit, then it's a load-store control dependencies?

I guess you missed the comma expression... Let me rewrite your pseudo-code
above, it is equivalent to

	if (work->node) {
		if ((work_next = work->node->next, true))
			clear_bit(&work->flags);
	}

another rewrite:
	
	if (work->node) {
		work_next = work->node->next;
		if ((work, true))
			clear_bit(&work->flags);
	}

and the final rewrite:

	if (work->node) {
		work_next = work->node->next;
		if (true)
			clear_bit(&work->flags);
	}

so again, I do not see the load-store control dependency. Not to mention this
code lacks READ_ONCE().


If we had something like

	if (work->node) {
		work_next = READ_ONCE(work->node->next);
		if (work_next)
			clear_bit(&work->flags);
	}

instead, then yes, we could rely on the LOAD-STORE dependency.

Oleg.