Skip site navigation (1)Skip section navigation (2)
Date:      Fri, 24 Sep 2010 10:25:11 -0400
From:      John Baldwin <jhb@freebsd.org>
To:        Kostik Belousov <kostikbel@gmail.com>
Cc:        svn-src-head@freebsd.org, svn-src-all@freebsd.org, pluknet <pluknet@gmail.com>, src-committers@freebsd.org, Dag-Erling Smorgrav <des@freebsd.org>
Subject:   Re: svn commit: r212723 - head/sys/compat/linprocfs
Message-ID:  <201009241025.11877.jhb@freebsd.org>
In-Reply-To: <20100924133215.GL34228@deviant.kiev.zoral.com.ua>
References:  <201009160756.o8G7uZrg065332@svn.freebsd.org> <201009240834.16786.jhb@freebsd.org> <20100924133215.GL34228@deviant.kiev.zoral.com.ua>

next in thread | previous in thread | raw e-mail | index | archive | help
On Friday, September 24, 2010 9:32:15 am Kostik Belousov wrote:
> On Fri, Sep 24, 2010 at 08:34:16AM -0400, John Baldwin wrote:
> > On Friday, September 24, 2010 7:53:11 am Kostik Belousov wrote:
> > > On Fri, Sep 24, 2010 at 02:17:29PM +0400, pluknet wrote:
> > > > On 16 September 2010 11:56, Dag-Erling Smorgrav <des@freebsd.org> 
wrote:
> > > > > Author: des
> > > > > Date: Thu Sep 16 07:56:34 2010
> > > > > New Revision: 212723
> > > > > URL: http://svn.freebsd.org/changeset/base/212723
> > > > >
> > > > > Log:
> > > > >  Implement proc/$$/environment.
> > > > >
> > > > [...]
> > > > 
> > > > >  /*
> > > > >  * Filler function for proc/pid/environ
> > > > >  */
> > > > >  static int
> > > > >  linprocfs_doprocenviron(PFS_FILL_ARGS)
> > > > >  {
> > > > > +       int ret;
> > > > >
> > > > > -       sbuf_printf(sb, "doprocenviron\n%c", '\0');
> > > > > -       return (0);
> > > > > +       PROC_LOCK(p);
> > > > 
> > > > With this change I observe the following sleepable after non-
sleepable:
> > > > 
> > > >  1st 0xffffff000290b558 process lock (process lock) @
> > > > /usr/src/sys/modules/linprocfs/../../compat/linprocfs/linprocfs.c:1049
> > > >  2nd 0xffffff00028f8848 user map (user map) @ 
> > /usr/src/sys/vm/vm_map.c:3525
> > > > KDB: stack backtrace:
> > > > db_trace_self_wrapper() at db_trace_self_wrapper+0x2a
> > > > _witness_debugger() at _witness_debugger+0x2e
> > > > witness_checkorder() at witness_checkorder+0x807
> > > > _sx_slock() at _sx_slock+0x55
> > > > vm_map_lookup() at vm_map_lookup+0x55
> > > > vm_fault() at vm_fault+0x113
> > > > proc_rwmem() at proc_rwmem+0x7a
> > > > linprocfs_doprocenviron() at linprocfs_doprocenviron+0x122
> > > > pfs_read() at pfs_read+0x45b
> > > > vn_read() at vn_read+0x256
> > > > dofileread() at dofileread+0xa1
> > > > kern_readv() at kern_readv+0x60
> > > > read() at read+0x55
> > > > syscallenter() at syscallenter+0x1aa
> > > > syscall() at syscall+0x4c
> > > > Xfast_syscall() at Xfast_syscall+0xe2
> > > > --- syscall (3, FreeBSD ELF64, read), rip = 0x80074134c, rsp =
> > > > 0x7fffffffe9c8, rbp = 0 ---
> > > > 
> > > > 
> > > > > +
> > > > > +       if ((ret = p_cansee(td, p)) != 0) {
> > > > > +               PROC_UNLOCK(p);
> > > > > +               return ret;
> > > > > +       }
> > > > > +
> > > > > +       ret = linprocfs_doargv(td, p, sb, ps_string_env);
> > > > > +       PROC_UNLOCK(p);
> > > > > +       return (ret);
> > > > >  }
> > > 
> > > This is easy to fix, isn't it ? But there seems to be much more nits.
> > > 
> > > First, allocating 512 * sizeof(char *)-byte object on the stack is not
> > > good.
> > > 
> > > Second, the initialization of iov_len for reading the array
> > > of string pointers misses '* sizeof(char *)'.
> > > 
> > > And third (probably fatal) is the lack of checks that the end of
> > > array and each string fits into the user portion of the map. I do not
> > > see why addr that already has u_long type is casted to u_long. Also,
> > > VM_MIN_ADDRESS, VM_MAXUSER_ADDRESS constants are for the native host
> > > FreeBSD ABI, they may differ from the target process limits.
> > > 
> > > Formatting is separate issue, let postpone it.
> > > 
> > > diff --git a/sys/compat/linprocfs/linprocfs.c 
> > b/sys/compat/linprocfs/linprocfs.c
> > > index c7fe158..fd62b1c 100644
> > > --- a/sys/compat/linprocfs/linprocfs.c
> > > +++ b/sys/compat/linprocfs/linprocfs.c
> > > @@ -952,12 +952,9 @@ linprocfs_doargv(struct thread *td, struct proc *p, 
> > struct sbuf *sb,
> > >  	struct uio tmp_uio;
> > >  	struct ps_strings pss;
> > >  	int ret, i, n_elements, found_end;
> > > -	u_long addr;
> > > -	char* env_vector[MAX_ARGV_STR];
> > > +	u_long addr, pbegin;
> > > +	char **env_vector;
> > >  	char env_string[UIO_CHUNK_SZ];
> > > -	char *pbegin;
> > > -
> > > -
> > >  
> > >  #define	UIO_HELPER(uio, iov, base, len, cnt, offset, sz, flg, rw, td)	
\
> > >  do {									\
> > > @@ -971,6 +968,10 @@ do {									\
> > >  	uio.uio_rw = (rw); 						\
> > >  	uio.uio_td = (td);						\
> > >  } while (0)
> > > +#define	VALID_USER_ADDR(addr)						\
> > > +	((addr) >= p->p_sysent->sv_minuser && (addr) < p->p_sysent-
>sv_maxuser)
> > > +
> > > +	env_vector = malloc(sizeof(char *) * MAX_ARGV_STR, M_TEMP, M_WAITOK);
> > >  
> > >  	UIO_HELPER(tmp_uio, iov, &pss, sizeof(struct ps_strings), 1,
> > >  	    (off_t)(p->p_sysent->sv_psstrings), sizeof(struct ps_strings),
> > > @@ -978,37 +979,41 @@ do {									\
> > >  
> > >  	ret = proc_rwmem(p, &tmp_uio);
> > >  	if (ret != 0)
> > > -		return ret;
> > > +		goto done;
> > >  
> > >  	/* Get the array address and the number of elements */
> > >  	resolver(pss, &addr, &n_elements);
> > >  
> > >  	/* Consistent with lib/libkvm/kvm_proc.c */
> > > -	if (n_elements > MAX_ARGV_STR || (u_long)addr < VM_MIN_ADDRESS ||
> > > -    	    (u_long)addr >= VM_MAXUSER_ADDRESS) {
> > > -		/* What error code should we return? */
> > > -		return 0;
> > > +	if (n_elements > MAX_ARGV_STR || !VALID_USER_ADDR(addr) ||
> > > +	    !VALID_USER_ADDR(addr + MAX_ARGV_STR * sizeof(char *))) {
> > > +		ret = EFAULT;
> > > +		goto done;
> > >  	}
> > >  
> > > - 	UIO_HELPER(tmp_uio, iov, env_vector, MAX_ARGV_STR, 1,
> > > + 	UIO_HELPER(tmp_uio, iov, env_vector, MAX_ARGV_STR * sizeof(char 
*), 1,
> > >  	    (vm_offset_t)(addr), iov.iov_len, UIO_SYSSPACE, UIO_READ, td);
> > >  
> > >  	ret = proc_rwmem(p, &tmp_uio);
> > >  	if (ret != 0)
> > > -		return ret;
> > > +		goto done;
> > >  
> > >  	/* Now we can iterate through the list of strings */
> > >  	for (i = 0; i < n_elements; i++) {
> > >  	    found_end = 0;
> > > -	    pbegin = env_vector[i];
> > > -		while(!found_end) {
> > > +	    pbegin = (vm_offset_t)env_vector[i];
> > > +	    while (!found_end) {
> > > +		    if (!VALID_USER_ADDR(pbegin) ||
> > > +			!VALID_USER_ADDR(pbegin + sizeof(env_string))) {
> > > +			    ret = EFAULT;
> > > +			    goto done;
> > > +		    }
> > >  		    UIO_HELPER(tmp_uio, iov, env_string, sizeof(env_string), 1,
> > > -			(vm_offset_t) pbegin, iov.iov_len, UIO_SYSSPACE,
> > > -			UIO_READ, td);
> > > +			pbegin, iov.iov_len, UIO_SYSSPACE, UIO_READ, td);
> > >  
> > >  			ret = proc_rwmem(p, &tmp_uio);
> > >  			if (ret != 0)
> > > -				return ret;
> > > +				goto done;
> > >  
> > >  			if (!strvalid(env_string, UIO_CHUNK_SZ)) {
> > >  			    /*
> > > @@ -1017,7 +1022,7 @@ do {									\
> > >  			     * the pointer
> > >  			     */
> > >  			    sbuf_bcat(sb, env_string, UIO_CHUNK_SZ);
> > > -			    pbegin = &(*pbegin) + UIO_CHUNK_SZ;
> > > +			    pbegin += UIO_CHUNK_SZ;
> > >  			} else {
> > >  			    found_end = 1;
> > >  			}
> > > @@ -1025,9 +1030,12 @@ do {									\
> > >  		sbuf_printf(sb, "%s", env_string);
> > >  	}
> > >  
> > > +#undef VALID_USER_ADDR
> > >  #undef UIO_HELPER
> > >  
> > > -	return (0);
> > > +done:
> > > +	free(env_vector, M_TEMP);
> > > +	return (ret);
> > >  }
> > >  
> > >  static void
> > > @@ -1052,9 +1060,11 @@ linprocfs_doprocenviron(PFS_FILL_ARGS)
> > >  		PROC_UNLOCK(p);
> > >  		return ret;
> > >  	}
> > > +	_PHOLD(p);
> > > +	PROC_UNLOCK(p);
> > >  
> > >  	ret = linprocfs_doargv(td, p, sb, ps_string_env);
> > > -	PROC_UNLOCK(p);
> > > +	PRELE(p);
> > >  	return (ret);
> > >  }
> > 
> > You need to add a P_WEXIT check here. _PHOLD() / PRELE() do not work
> > once P_WEXIT is set (rather, exit1() may not notice and wait for a
> > _PHOLD() invoked to drain once P_WEXIT is set).
> 
> Hmm, I think that PHOLD is not needed there, actually. Pseudofs
> checks for exiting process in pfs_visible(_proc). Before the fill
> method is called, process is held and unlocked. So my second _PHOLD
> could be removed.

Yes, I think that is true.

-- 
John Baldwin



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201009241025.11877.jhb>