Skip to contentSkip to conversation

against mmap.c (do_mmap_pgoff) and a note

PATCH RESEND In review DervishD 2 participants 4 messages May 27, 2003 – Jun 16, 2003 1 message not in the archive

Subsystems (2)
Memory management Memory mapping
lore
Share & tools
lore

DervishD

author
#0 lore raw

PATCH RESEND 1/1 against mmap.c (do_mmap_pgoff) and a note

Hi Linus :)

I've noticed that in file 'mm/mmap.c', at the comments for
function 'can_vma_merge_before' this can be read:

<quote>
* We don't check here for the merged mmap wrapping around the end of pagecache
* indices (16TB on ia32) because do_mmap_pgoff() does not permit mmap's which
* wrap, nor mmaps which cover the final page at index -1UL.
*/
</quote>

Well, the patch I sent you against 2.5.60 and that I'm resending,
since it's not included at 2.5.68, fixed a mmap() corner case bug, that made mmap() fail for large sizes (as did a previous patch that you did apply) and that works even if 'TASK_SIZE' is the entire address space (as seems the case at sparc64, for example). The problem is that with the patch you already applied and with the new patch I send, you cannot do a mmap that wraps, but you can mmap the final page (not at index -1UL, there is not such index!, -1 unsigned?). In fact, you can mmap your entire address space if you want.

Without the patch you already applied any such 'big' mmap will
silently fail, and with the new, if the arch doesn't want such a big mmap it will return EINVAL.

I think the problem in 'can_vma_merge_before' is just the comment
(for me it sounds ambiguous), but the other problem, with archs like sparc64, still is an issue. Dave Miller pointed this although I made the patch. Please apply, the semantics of 'do_mmap_pgoff' doesn't change a bit, is just that the current code will silently fail for sparc64 and other archs where TASK_SIZE is the entire address space.

Thanks a lot and happy coding :)

Raúl Núñez de Arenas Coronado

Signature · 2 lines
Linux Registered User 88736
http://www.pleyades.net & http://raul.pleyades.net/
mm/mmap.c renamed from mm/mmap.c.orig +4 -4
@@ -421,14 +421,14 @@
 	if (file && (!file->f_op || !file->f_op->mmap))
 		return -ENODEV;
 
-	if (!len)
+	if (len == 0)
 		return addr;
 
-	if (len > TASK_SIZE)
-		return -EINVAL;
-
 	len = PAGE_ALIGN(len);
 
+	if (len > TASK_SIZE || len == 0)
+		return -EINVAL;
+
 	/* offset overflow? */
 	if ((pgoff + (len >> PAGE_SHIFT)) < pgoff)
 		return -EINVAL;

A message that is not in the archive (sent off-list or not yet received): [email protected]

author
#2 lore raw

* Jörn Engel <[email protected]> dixit:

Quoted 3 lines

mm/mmap.c line 424

Quoted 2 lines
mm/mmap.c renamed from mm/mmap.c.orig +1 -1
-	if (!len)
+	if (len == 0)

No change.

Just cosmethic, ask Dave S. Miller, he is the author of the
change, I'm just doing the patch for him.

Quoted 7 lines
Quoted 7 lines
mm/mmap.c renamed from mm/mmap.c.orig +0 -3
 		return addr;
 
-	if (len > TASK_SIZE)
-		return -EINVAL;
-
 	len = PAGE_ALIGN(len);
Quoted 8 lines

mm/mmap.c line 431

Quoted 3 lines
mm/mmap.c renamed from mm/mmap.c.orig +3 -0
+	if (len > TASK_SIZE || len == 0)
+		return -EINVAL;
+

PAGE_ALIGN(0) = 0
PAGE_ALIGN(1) = PAGE_SIZE

Again, no change.

There is a change in archs where TASK_SIZE is the entire
addressable space (like sparc64). Ask Dave S., again. The problem did arise when TASK_SIZE is ~0. Then semantics change.

Raúl Núñez de Arenas Coronado

Signature · 2 lines
Linux Registered User 88736
http://www.pleyades.net & http://raul.pleyades.net/
#3 lore raw
Quoted 16 lines · DervishD

* Jörn Engel <[email protected]> dixit:

Quoted 14 lines

mm/mmap.c line 431

Quoted 10 lines
mm/mmap.c renamed from mm/mmap.c.orig +3 -3
 		return addr;
 
-	if (len > TASK_SIZE)
-		return -EINVAL;
-
 	len = PAGE_ALIGN(len);
 
+	if (len > TASK_SIZE || len == 0)
+		return -EINVAL;
+

PAGE_ALIGN(0) = 0
PAGE_ALIGN(1) = PAGE_SIZE

Quoted 5 lines
Quoted 1 line

Again, no change.

There is a change in archs where TASK_SIZE is the entire
addressable space (like sparc64). Ask Dave S., again. The problem did arise when TASK_SIZE is ~0. Then semantics change.

True. PAGE_ALIGN(-1) = 0 and that case would not get caught with the old code. Looks good to me.

Jörn

Signature · 4 lines
Public Domain  - Free as in Beer
General Public - Free as in Speech
BSD License    - Free as in Enterprise
Shared Source  - Free as in "Work will make you..."
author
#4 lore raw

Hi Jörn :)

* Jörn Engel <[email protected]> dixit:

Quoted 5 lines
Quoted 3 lines

There is a change in archs where TASK_SIZE is the entire
addressable space (like sparc64). Ask Dave S., again. The problem did arise when TASK_SIZE is ~0. Then semantics change.

True. PAGE_ALIGN(-1) = 0 and that case would not get caught with the old code. Looks good to me.

This was pointed to me by Dave. I prepared a patch time ago just
for the case where 'len' was quite large, but I assumed that TASK_SIZE was at least one less page than the entire addressable space, which is not true. My fault O:))

Thanks for pointing, anyway.

Quoted 4 lines

Public Domain - Free as in Beer
General Public - Free as in Speech
BSD License - Free as in Enterprise
Shared Source - Free as in "Work will make you..."

Good one ;))))

Raúl Núñez de Arenas Coronado

Signature · 2 lines
Linux Registered User 88736
http://www.pleyades.net & http://raul.pleyades.net/