Re: Linux 2.4.2-ac27

[Martin Diehl]

(Linus cc'ed - related thread: 243-pre[78]: mmap changes (breaks) /proc)

On Wed, 28 Mar 2001, Alan Cox wrote:

> 2.4.2-ac27
> o	Revert mmap change that broke assumptions (and	(Martin Diehl)
> 	it seems SuS) 

the reason to suggest keeping the test was not due to len=0 behaviour of
mmap in general as is suggested by your comment. The breakage that I've
seen was due to mmap not returning -ENODEV for files from /proc despite
the lack of valid f_op->mmap (because the test was moved behind the len==0
check). The point is sed(1) first tries to mmap(2) the file and falls back
to read(2) in case of -ENODEV (and probably other errors too). This is
important for /proc since most files there are stat'ed size=0 but return
stuff when reading. Not getting error for mmap len=0 file makes sed behave
like at EOF. Anyway, reverting it was not addressed to cases where
f_op->mmap is valid but request is to mmap len=0 - we still return the 
startaddr parameter in that case:

$ touch nullfile
$ strace sed 's/./X/' nullfile
open("nullfile", O_RDONLY|O_LARGEFILE)  = 4
fstat64(4, {st_mode=S_IFREG|0644, st_size=0, ...}) = 0
mmap2(NULL, 0, PROT_READ, MAP_PRIVATE, 4, 0) = 0

This is consistent throughout all 2.4.x at least. From your comment I've
learnt SuS v2 requires -ENODEV for the len=0 case. While this would
resolve the /proc issue as well there might be some chance to brake code
which expects mmap(len=0) to succeed.
BTW, man-pages (1.31) say, mmap(2) returns -EINVAL if called with bad
start/length/offset values - but makes no claims whether len=0 would be
valid or not.

In case we want to follow what you've said about SuS, the right thing
might simply go along

--- linux-243-pre8/mm/mmap.c	Wed Mar 28 13:14:19 2001
+++ linux-243p8-md/mm/mmap.c	Thu Mar 29 09:49:34 2001
@@ -204,8 +204,12 @@
 	int correct_wcount = 0;
 	int error;
 
+	/* We need to error mmaps of 0 length. The apps rely on this and
+	   SuS v2 says that we return -ENODEV in this case without mentioning
+	   returning 0 for 0 length mmap */
+
 	if ((len = PAGE_ALIGN(len)) == 0)
-		return addr;
+		return -ENODEV;
 
 	if (len > TASK_SIZE || addr > TASK_SIZE-len)
 		return -EINVAL;

I really have no idea about what, if any, code might rely on old
beahviour. Some commonly used tools may get confused with stuff from
/var/lock/subsys tree for example. So probably it's better not to field
this change before 2.5 to be able to identify such things in time.
In this case we could add some clarification to your comment, saying that
returning error on len=0 is a future thing, leave the current len=0
semantics untouched, keep the f_op->mmap test at the very beginning (and
drop the moved one a few lines below).

Regards
Martin