Re: [PATCH] modules: fix livelock in add_unformed_module()

From: Jessica Yu
Date: Tue May 28 2019 - 10:50:29 EST


+++ Prarit Bhargava [28/05/19 10:30 -0400]:


On 5/22/19 1:08 PM, Prarit Bhargava wrote:


On 5/13/19 10:37 AM, Barret Rhoden wrote:
Hi -


Hey Barret, my apologies for not getting back to you earlier. I got caught up
in something that took me away from this issue.

On 5/13/19 7:23 AM, Prarit Bhargava wrote:
[snip]
A module is loaded once for each cpu.

Does one CPU succeed in loading the module, and the others fail with EEXIST?

My follow-up patch changes from wait_event_interruptible() to
wait_event_interruptible_timeout() so the CPUs are no longer sleeping and can
make progress on other tasks, which changes the return values from
wait_event_interruptible().

https://marc.info/?l=linux-kernel&m=155724085927589&w=2

I believe this also takes your concern into account?

That patch might work for me, but I think it papers over the bug where the check
on old->state that you make before sleeping (was COMING || UNFORMED, now !LIVE)
doesn't match the check to wake up in finished_loading().

The reason the issue might not show up in practice is that your patch basically
polls, so the condition checks in finished_loading() are only a quicker exit.

If you squash my patch into yours, I think it will cover that case. Though if
polling is the right answer here, it also raises the question of whether or not
we even need finished_loading().


The more I look at this I think you're right. Let me do some additional testing
with your patch + my original patch.


I have done testing on arm64, s390x, ppc64le, ppc64, and x86 and have not seen
any issues.

Jessica, how would you like me to proceed? Would you like an updated patch with
Signed-off's from both Barret & myself?

Hi Prarit,

A freshly sent patch with the squashed changes and Signed-off's would
be great. Thank you both for testing and looking into this!

Jessica