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

[Chagin Dmitry]

On Thu, Mar 26, 2009 at 05:08:59PM -0700, Doug Ambrisko wrote:
> 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?
> 


yes, you are right. s/uint32_t/l_ulong/ :)
also remove __packed.
thnx!


> 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.

-- 
Have fun!
chd
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 196 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/svn-src-all/attachments/20090327/7ccb4f6c/attachment.pgp