svn commit: r190445 - in head/sys: amd64/linux32 compat/linprocfs compat/linux conf dev/ipmi modules/ipmi modules/linprocfs

[Doug Ambrisko]

Doug Ambrisko writes:
| John Baldwin writes:
| | On Thursday 26 March 2009 5:29:42 pm Doug Ambrisko wrote:
| 	[snip]
| | > Maybe you have another suggestion to fix this.  The problem showed up
| | > when doing a mmap of 0xcf79c000 into 0xffffffffcf79c000 also a mmap
| | > of 0xf0000 ended up the same way.  This caused it to fail.  Note this
| | > is only on amd64 with a Linux.  It didn't happen with a FreeBSD i386
| | > version on amd64.  Here is a sample test program:
| | 
| | I'm sure this can be easily fixed in the Linux mmap() handlers instead.  Do 
| | you know if your Linux binary is using mmap2() or the old mmap()?
| 
| I think it uses linux_mmap then bouncing it to linux_mmap_common.
| linux_mmap_common had it right but when it mmap picked it up then 
| it was wrong in my intrumentation. 
| 
| I'll flip the l_off_t type back and then instrument it more to find
| out when things are going bad.  I missed the other usage of l_off_t
| so I agree this is a bad change.  However, I wonder if the other
| usage of l_off_t actually works right or there is a bug with that
| as well?
| 
| I should be able to get something put together pretty quick and
| send it for review.

Okay, I did some more instrumenting again and found that I was 
slightly wrong.  The mmap that was failing was 0xcf79c000 and not
0xf0000.  This probably makes since since the sign bit was set
on 0xcf79c000.  However, it appear mmap doesn't really do negative
seeks.  Looking at the freebsd32_mmap the structure it uses for
args is:
  struct freebsd6_freebsd32_mmap_args {
        char addr_l_[PADL_(caddr_t)]; caddr_t addr; char addr_r_[PADR_(caddr_t)];
        char len_l_[PADL_(size_t)]; size_t len; char len_r_[PADR_(size_t)];
        char prot_l_[PADL_(int)]; int prot; char prot_r_[PADR_(int)];
        char flags_l_[PADL_(int)]; int flags; char flags_r_[PADR_(int)];
        char fd_l_[PADL_(int)]; int fd; char fd_r_[PADR_(int)];
        char pad_l_[PADL_(int)]; int pad; char pad_r_[PADR_(int)];
        char poslo_l_[PADL_(u_int32_t)]; u_int32_t poslo; char poslo_r_[PADR_(u_int32_t)];
        char poshi_l_[PADL_(u_int32_t)]; u_int32_t poshi; char poshi_r_[PADR_(u_int32_t)];
  };
with both the high and the lows being u_int32_t.

So I wonder if in the linux32 the structure that is:
  struct l_mmap_argv {
        l_uintptr_t     addr;
        l_size_t        len;
        l_int           prot;
        l_int           flags;
        l_int           fd;
        l_off_t         pgoff;
  } __packed;
should be uint32_t for pgoff?

Using this patch things work okay:

Index: linux.h
===================================================================
RCS file: /usr/local/cvsroot/freebsd/src/sys/amd64/linux32/linux.h,v
retrieving revision 1.24
diff -u -p -r1.24 linux.h
--- linux.h	26 Mar 2009 17:14:22 -0000	1.24
+++ linux.h	27 Mar 2009 00:01:07 -0000
@@ -79,7 +79,7 @@ typedef l_ulong		l_ino_t;
 typedef l_int		l_key_t;
 typedef l_longlong	l_loff_t;
 typedef l_ushort	l_mode_t;
-typedef l_ulong		l_off_t;
+typedef l_long		l_off_t;
 typedef l_int		l_pid_t;
 typedef l_uint		l_size_t;
 typedef l_long		l_suseconds_t;
Index: linux32_machdep.c
===================================================================
RCS file: /usr/local/cvsroot/freebsd/src/sys/amd64/linux32/linux32_machdep.c,v
retrieving revision 1.52
diff -u -p -r1.52 linux32_machdep.c
--- linux32_machdep.c	18 Feb 2009 16:11:39 -0000	1.52
+++ linux32_machdep.c	27 Mar 2009 00:01:07 -0000
@@ -788,6 +788,7 @@ linux_mmap(struct thread *td, struct lin
 {
 	int error;
 	struct l_mmap_argv linux_args;
+	uint32_t pos;
 
 	error = copyin(args->ptr, &linux_args, sizeof(linux_args));
 	if (error)
@@ -801,7 +802,10 @@ linux_mmap(struct thread *td, struct lin
 #endif
 	if ((linux_args.pgoff % PAGE_SIZE) != 0)
 		return (EINVAL);
-	linux_args.pgoff /= PAGE_SIZE;
+	pos = linux_args.pgoff;
+	pos /= PAGE_SIZE;
+	linux_args.pgoff = pos;
+	
 
 	return (linux_mmap_common(td, &linux_args));
 }


So which should we do?  The uint32_t for the /= PAGE_SIZE or in
the mmap structure?  FWIW, they are mmaping /dev/mem and grabbing
the SMBIOS structure put at 0xcf79c000 and 0xcf7f0000 which are not
negative offsets.  linux_mmap2 and linux_common don't really have
this problem in this case since they are using the memory address 
/ PAGE_SIZE.  So they don't run into this sign problem like this.
I've confirmed that that above patch makes the Linux BMC firmware
upgrade tool works.  On a real Linux machine it also mmaps these
addresses and it works there otherwise the program goes into the
weeds since it can't find the IPMI controller.  This change only
mucks with linux_mmap.

Thanks,

Doug A.