Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 4 Aug 2022 15:06:21 +0200
From:   Petr Mladek <pmladek@suse.com>
To:     Rik van Riel <riel@surriel.com>
Cc:     Josh Poimboeuf <jpoimboe@kernel.org>, linux-kernel@vger.kernel.org,
        live-patching@vger.kernel.org, kernel-team@fb.com,
        Jiri Kosina <jikos@kernel.org>,
        Miroslav Benes <mbenes@suse.cz>,
        Joe Lawrence <joe.lawrence@redhat.com>,
        Breno Leitao <leitao@debian.org>
Subject: Re: [PATCH v4] livepatch: fix race between fork and KLP transition
Message-ID: <YuvETeDi56Uv22nS@alley>
References: <20220720121023.043738bb@imladris.surriel.com>
 <YtrCqMLUqJlcoqIo@alley>
 <20220722150106.683f3704@imladris.surriel.com>
 <Yt6bZo5ztnVSjLLC@alley>
 <20220725094919.52bcde19@imladris.surriel.com>
 <20220727001040.vlqnnb4a3um46746@treble>
 <20220727102437.34530586@imladris.surriel.com>
 <YuKtNtOshtRfeMn7@alley>
 <c04f97fc29c4618f137b27ce6537800b53f1d95f.camel@surriel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <c04f97fc29c4618f137b27ce6537800b53f1d95f.camel@surriel.com>
Precedence: bulk

On Tue 2022-08-02 16:07:08, Rik van Riel wrote:
> On Thu, 2022-07-28 at 17:37 +0200, Petr Mladek wrote:
> > On Wed 2022-07-27 10:24:37, Rik van Riel wrote:
> > > v4: address changelog comments by Josh (thank you)
> > > 
> > > ---8<---
> > > When a KLP fails to apply, klp_reverse_transition will clear the
> > > TIF_PATCH_PENDING flag on all tasks, except for newly created tasks
> > > which are not on the task list yet.
> > 
> > It actually is not true. klp_reverse_transtion() clears
> > TIF_PATCH_FLAG only
> > temporary when it waits until all processes leave the ftrace
> > handler. It sets TIF_PATCH_FLAG once again for all tasks by calling
> > klp_start_transition().
> > 
> > The difference is important. The WARN_ON_ONCE() in
> > klp_complete_transition() will be printed when fork() copied
> > TIF_PATCH_FLAG before it was set again.
> > 
> > Anyway, the important thing is that TIF_PATCH_FLAG and task-
> > >patch_state
> > might be incompatible because fork() copies them at different times.
> > 
> > klp_copy_process() must make sure that they are in sync. And
> > it must be done under tasklist_lock when the child is added
> > to the global task list.
> 
> Hmmm, how should this be addressed in the changelog?
> 
> Should I just remove most of that paragraph and leave it
> at "there can be a race"?

It would be nice to somehow summarize what I wrote. I mean to explain
why the problem is easier to see with revert and not with forward
transition.

It is because TIF_PATCH_FLAG might stay cleared in the child even
when it was set again in the parent by the klp_revert_transtion().
As a result, the child will never get transition back to the reverted
state.

The problem is hard to hit during the forward transition because
child might have TIF_PATCH_FLAG still set even when
it might later copy an already migrated task->patch_state
when parent gets migrated in the race window. In this case,
the TIF_PATCH_FLAG will get cleared when the child returns
from fork and all will be good.

In each case, the inconsistent state is there even during
the forward transition. But it would be caught only when
the entire transition is finished during the rather small
race window.

The patch should fix the race in any direction.

I could provide even better description after I am back
from vacation on Aug 22.

Best Regards,
Petr