Message-ID: <szuouie2gbpaj6gynixelasgeo5fxtn5fd3vbmebzve2x3auum@2q4cjchfajvh>
Date: Thu, 15 Aug 2024 12:49:49 -0400
From: "Liam R. Howlett" <Liam.Howlett@...cle.com>
To: Jeff Xu <jeffxu@...omium.org>
Cc: akpm@...ux-foundation.org, willy@...radead.org,
        torvalds@...ux-foundation.org, pedro.falcato@...il.com,
        linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org,
        linux-mm@...ck.org, linux-hardening@...r.kernel.org, jeffxu@...gle.com,
        lorenzo.stoakes@...cle.com, mpe@...erman.id.au, oliver.sang@...el.com,
        vbabka@...e.cz, keescook@...omium.org
Subject: Re: [PATCH v1 0/2] mremap refactor: check src address for vma
 boundaries first.

* Jeff Xu <jeffxu@...omium.org> [240814 23:46]:
> On Wed, Aug 14, 2024 at 12:55 PM Liam R. Howlett
> <Liam.Howlett@...cle.com> wrote:
> > The majority of the comments to V2 are mine, you only told us that
> > splitting a sealed vma is wrong (after I asked you directly to answer)
> > and then you made a comment about testing of the patch set. Besides the
> > direct responses to me, your comment was "wait for me to test".
> >
> Please share this link for  " Besides the direct responses to me, your
> comment was "wait for me to test".
> Or  pop up that email by responding to it, to remind me.  Thanks.

[1].

> 
> > You are holding us hostage by asking for more testing but not sharing
> > what is and is not valid for mseal() - or even answering questions on
> > tests you run.
> https://docs.kernel.org/process/submitting-patches.html#don-t-get-discouraged-or-impatient

If you are implying that I'm impatient, I can assure you that is not
the feeling driving these emails.

You are just trying to push a patch through that changes the exact code
that you said you would test but didn't say how, and you said the
testing of another patch was insufficient but didn't say why.  Then you
send out this fix.

> 
> > These patches should be rejected in favour of fixing the feature like it
> > should have been written in the first place.
> This is not ture.

Yes, it is.

> 
> Without removing arch_unmap, it is impossible to implement in-loop.

arch_unmap() is going away, besides..

arch_unmap() could fail today and leave the ppc vdso pointing to NULL,
mseal() would introduce a even less likely case of this happening.  I
asked you about this in v10 [2].  I elaborated in my response, but I
doubt you got that far in the email.

> And I have mentioned this during initial discussion of mseal patch, as
> well as when Pedro expressed the interest on in-loop approach.  If you
> like reference, I can find the links for you.

So the main concern is that ppc is going to mseal the vdso, then fail to
unmap it?

It would have been better to put a check in the arch_unmap() code in ppc
to avoid that - but it will never happen.

> 
> I'm glad that arch_unmap is removed now and resulting in much cleaner
> code, 

If you care at all about cleaner code, please move the mseal check to
where it should be - or stop getting in the way of others moving it.

> it has always been a question/mysterial to me ever since I read
> that code.

You could have also looked into what arch_unmap() did, or why it was
where it is today.  If you had, you would have found that arch_unmap()
could be moved lower in the function and allowed in-loop approach - but
you didn't bother to find out what it was about.

Liam

...

[1]. https://lore.kernel.org/all/CALmYWFs0v07z5vheDt1h3hD+3--yr6Va0ZuQeaATo+-8MuRJ-g@mail.gmail.com/
[2]. https://lore.kernel.org/lkml/3rpmzsxiwo5t2uq7xy5inizbtaasotjtzocxbayw5ntgk5a2rx@jkccjg5mbqqh/

Powered by blists - more mailing lists