[2.6.22.y] {01**/17} - do_anonymous_page-race - series for stable kernel

[2.6.22.y] {01**/17} - do_anonymous_page-race - series for stable kernel

[Oliver Pinter (Pintér Olivér)]

[-- Attachment #1: Type: text/plain, Size: 65 bytes --]

NOT IN MAINLINE

Linus it's go or drop it?



-- 
Thanks,
Oliver

[-- Attachment #2: do_anonymous_page-race --]
[-- Type: message/rfc822, Size: 987 bytes --]



ZGl0aW9uIGluIHVzZXJzcGFjZSB0ZXN0Y2FzZQpSZWZlcmVuY2VzOiA0Njk0OCwgTFRDMTE1NzQK
CgpBZGRpdGlvbmFsIENvbW1lbnQgIzEwMyBGcm9tIEFuZHJlYSBBcmNhbmdlbGkgMjAwNC0xMC0x
NSAxOTo0MQp0aGUgbGFzdCBwYXRjaCBJIGF0dGFjaGVkIGlzIHRoZSBzYWZlc3QgSSBiZWxpZXZl
LgoKSSdtIG5vdCBzdXJlIGlmIGEgbG9ja191bmxvY2sgb3IgbG9ja191bmxvY2sgaXMgYWx3YXlz
IGd1YXJhbnRlZWQgdG8gaGFwcGVuCmFmdGVyIHRoZSBJL08sIGFuZCB0aGlzIG1ha2VzIHN1cmUg
bm8gcmFjZSBjYW4gaGFwcGVuIGFueW1vcmUuCgotLS0KIGZzL2Jpby5jIHwgICAgOSArKysrKysr
KysKIDEgZmlsZSBjaGFuZ2VkLCA5IGluc2VydGlvbnMoKykKCi0tLSBhL2ZzL2Jpby5jCTIwMDct
MDctMDggMTk6MzI6MTcuMDAwMDAwMDAwIC0wNDAwCisrKyBiL2ZzL2Jpby5jCTIwMDctMDgtMjcg
MTQ6MDE6MjEuMDAwMDAwMDAwIC0wNDAwCkBAIC0xMDI4LDYgKzEwMjgsMTUgQEAgdm9pZCBiaW9f
ZW5kaW8oc3RydWN0IGJpbyAqYmlvLCB1bnNpZ25lZAogCWJpby0+Ymlfc2l6ZSAtPSBieXRlc19k
b25lOwogCWJpby0+Ymlfc2VjdG9yICs9IChieXRlc19kb25lID4+IDkpOwogCisJaWYgKGJpb19k
YXRhX2RpcihiaW8pID09IFJFQUQpCisJCS8qCisJCSAqIElmIHRoZSBjdXJyZW50IGNwdSBoYXMg
d3JpdHRlbiB0byB0aGUgcGFnZSBieSBoYW5kCisJCSAqIHdpdGhvdXQgZG1hLCB3ZSBtdXN0IGVu
Zm9yY2Ugb3JkZXJpbmcgdG8gYmUgc3VyZQorCQkgKiB0aGlzIHdyaXR0ZW4gZGF0YSB3aWxsIGJl
IHZpc2libGUgYmVmb3JlIHdlIGV4cG9zZQorCQkgKiB0aGUgcGFnZSBjb250ZW50cyB0byBvdGhl
ciBjcHVzIChmb3IgZXhhbXBsZSB3aXRoCisJCSAqIGEgc2V0X3B0ZSkuCisJCSAqLworCQlzbXBf
d21iKCk7CiAJaWYgKGJpby0+YmlfZW5kX2lvKQogCQliaW8tPmJpX2VuZF9pbyhiaW8sIGJ5dGVz
X2RvbmUsIGVycm9yKTsKIH0K

Re: [2.6.22.y] {01**/17} - do_anonymous_page-race - series for stable kernel

[Linus Torvalds]

On Sat, 2 Feb 2008, Oliver Pinter (Pint?r Oliv?r) wrote:
>
> NOT IN MAINLINE
> 
> Linus it's go or drop it?

I have no idea, because you've used some horrible and stupid attachment 
format that I can't even read. Patches should be inline so that people can 
see them and comment on them.

		Linus

Re: [2.6.22.y] {01**/17} - do_anonymous_page-race - series for stable kernel

[Hugh Dickins]

On Sat, 2 Feb 2008, Linus Torvalds wrote:
> On Sat, 2 Feb 2008, Oliver Pinter (Pint?r Oliv?r) wrote:
> >
> > NOT IN MAINLINE
> > 
> > Linus it's go or drop it?
> 
> I have no idea, because you've used some horrible and stupid attachment 
> format that I can't even read. Patches should be inline so that people can 
> see them and comment on them.

Here it is, below, from a more accessible earlier posting by Oliver.
I'd say that this is clearly not suitable to be rushed into stable
ahead of mainline (though it wouldn't do worse than tiny slowdown).

(a) It's from three years ago, what's changed around meanwhile?
(b) It's a mod to bio_endio(), I don't see how it can relate to
    the title's unexplained "do_anonymous_page race" (anonymous?).
(c) If this adds an smp_wmb, where's the corresponding smp_rmb?
(d) Comment doesn't even make sense: a lock_unlock or lock_unlock?

I suspect it relates to the smp_wmb's 2.6.9 added to clear_user_highpage
and copy_user_highpage around the same time (the ones which Nick wishes
to move to SetPageUptodate); maybe that was mainline's preferred answer
to the same problem.

Andrea, if this is still needed, I think it'd be best if you send a
proper description of the problem and argument for your solution to
Andrew cc Nick and Jens, after that it could be considered for stable.

Thanks,
Hugh (who was intrigued by the "do_anonymous_page race" title)

commit 8327e0f191341cc32fb89bf4d7bee7c2524ae4e0
Author: Andrea Arcangeli <andrea@suse.de>
Date:   Mon Jan 28 20:52:29 2008 +0100

    Subject: Race condition in userspace testcase
    References: 46948, LTC11574
    
    
    Additional Comment #103 From Andrea Arcangeli 2004-10-15 19:41
    the last patch I attached is the safest I believe.
    
    I'm not sure if a lock_unlock or lock_unlock is always guaranteed to happen
    after the I/O, and this makes sure no race can happen anymore.

diff --git a/fs/bio.c b/fs/bio.c
index 093345f..984d589 100644
--- a/fs/bio.c
+++ b/fs/bio.c
@@ -1028,6 +1028,15 @@ void bio_endio(struct bio *bio, unsigned int bytes_done, int error)
 	bio->bi_size -= bytes_done;
 	bio->bi_sector += (bytes_done >> 9);
 
+	if (bio_data_dir(bio) == READ)
+		/*
+		 * If the current cpu has written to the page by hand
+		 * without dma, we must enforce ordering to be sure
+		 * this written data will be visible before we expose
+		 * the page contents to other cpus (for example with
+		 * a set_pte).
+		 */
+		smp_wmb();
 	if (bio->bi_end_io)
 		bio->bi_end_io(bio, bytes_done, error);
 }

Re: [2.6.22.y] {01**/17} - do_anonymous_page-race - series for stable kernel

[Oliver Pinter (Pintér Olivér)]

On 2/2/08, Linus Torvalds <torvalds@linux-foundation.org> wrote:
>
>
> On Sat, 2 Feb 2008, Oliver Pinter (Pint?r Oliv?r) wrote:
> >
> > NOT IN MAINLINE
> >
> > Linus it's go or drop it?
>
> I have no idea, because you've used some horrible and stupid attachment
> format that I can't even read. Patches should be inline so that people can
> see them and comment on them.
>
> 		Linus
>

sorry, then im future im switched to plain text mails


-- 
Thanks,
Oliver

Re: [2.6.22.y] {01**/17} - do_anonymous_page-race - series for stable kernel

[Oliver Pinter]

---8<---

-- 
Thanks,From: Andrea Arcangeli <andrea@suse.de>
Subject: Race condition in userspace testcase
References: 46948, LTC11574


Additional Comment #103 From Andrea Arcangeli 2004-10-15 19:41
the last patch I attached is the safest I believe.

I'm not sure if a lock_unlock or lock_unlock is always guaranteed to happen
after the I/O, and this makes sure no race can happen anymore.

---
 fs/bio.c |    9 +++++++++
 1 file changed, 9 insertions(+)

--- a/fs/bio.c	2007-07-08 19:32:17.000000000 -0400
+++ b/fs/bio.c	2007-08-27 14:01:21.000000000 -0400
@@ -1028,6 +1028,15 @@ void bio_endio(struct bio *bio, unsigned
 	bio->bi_size -= bytes_done;
 	bio->bi_sector += (bytes_done >> 9);

+	if (bio_data_dir(bio) == READ)
+		/*
+		 * If the current cpu has written to the page by hand
+		 * without dma, we must enforce ordering to be sure
+		 * this written data will be visible before we expose
+		 * the page contents to other cpus (for example with
+		 * a set_pte).
+		 */
+		smp_wmb();
 	if (bio->bi_end_io)
 		bio->bi_end_io(bio, bytes_done, error);
 }
---8<---

--
Oliver