Re: CVS commit: src/sys/kern

[Chuck Silvers <chuq%chuq.com@localhost>]

On Sun, Oct 06, 2019 at 08:41:35AM +0200, Maxime Villard wrote:
> Le 01/10/2019 à 18:36, Chuck Silvers a écrit :
> > Module Name:	src
> > Committed By:	chs
> > Date:		Tue Oct  1 16:36:58 UTC 2019
> > 
> > Modified Files:
> > 	src/sys/kern: sysv_shm.c
> > 
> > Log Message:
> > in shmdt(), wait until shmat() completes before detaching.
> > 
> > Reported-by: syzbot+8f470a1bf36b47ae0040%syzkaller.appspotmail.com@localhost
> > Reported-by: syzbot+45810b4c41ed65d9148d%syzkaller.appspotmail.com@localhost
> > 
> > 
> > To generate a diff of this commit:
> > cvs rdiff -u -r1.138 -r1.139 src/sys/kern/sysv_shm.c
> > 
> > Please note that diffs are not public domain; they are subject to the
> > copyright notices on the relevant files.
> 
> Looks like this line is missing
> 
> diff --git a/sys/kern/sysv_shm.c b/sys/kern/sysv_shm.c
> index b090d1bc4262..c52cb65ec768 100644
> --- a/sys/kern/sysv_shm.c
> +++ b/sys/kern/sysv_shm.c
> @@ -259,6 +259,7 @@ shmmap_getprivate(struct proc *p)
>  		shmmap_se = kmem_alloc(sizeof(struct shmmap_entry), KM_SLEEP);
>  		shmmap_se->va = oshmmap_se->va;
>  		shmmap_se->shmid = oshmmap_se->shmid;
> +		shmmap_se->busy = oshmmap_se->busy;
>  		SLIST_INSERT_HEAD(&shmmap_s->entries, shmmap_se, next);
>  	}
>  	shmmap_s->nitems = oshmmap_s->nitems;
> 
> The access made from the caller is not initialized
> 
> 353 	if (shmmap_se->busy) {


You are right that my change left the new field uninitialized in this case,
but unfortunately fixing it is not as simple as that.
if the shmmap_s is unshared while one of the entries is busy
then that entry would stay busy forever in the copy that is not updated
by the thread that dropped shm_lock in the middle of shmat().

I'm going to take the simple way out for now and undo the previous change
and instead change shmat() to just hold shm_lock until it is completely done.

-Chuck