On Mon, Jan 23, 2017 at 08:42:07PM +1000, Martin Pieuchot wrote:
> On 17/01/17(Tue) 00:24, Alexander Bluhm wrote:
> > On Mon, Jan 16, 2017 at 08:36:46PM +0100, [email protected] wrote:
> > > kernel: protection fault trap, code=0
> > > Stopped at fd_getfile+0x20: testb $0x2,mptramp_gdt32_desc+0x1e(%r
> > > ax)
> > > ddb{3}> fd_getfile() at fd_getfile+0x20
> > > sys_fstat() at sys_fstat+0x43
> > > syscall() at syscall+0x27b
> >
> > It crashes in fd_getfile() FILE_IS_USABLE(fp) as fdp->fd_ofiles has
> > been freed.
>
> Are you sure? The faulting instruction is:
>
> /sys/kern/kern_descrip.c:190
> 80: f6 40 40 02 testb $0x2,0x40(%rax)
> 84: 75 e7 jne 6d <fd_getfile+0xd>
>
> So %rax contains an incorrect value which is not NULL, are you
> suggesting that it is garbage due to free(9) poisoning the memory?
Yes, FILE_IS_USABLE(fp) checks FIF_LARVAL and that is 0x02, and
f_iflags is at offset 64 == 0x40.
> If that's the case, the easiest fix in would be to do the allocations
> upfront. An alternative solution discussed here at a2k17 would be to
> use a SRP to guarantee that fd_ofiles is not freed while another CPU is
> still referencing it. That would also help for MP work.
As this is not an MP problem, I would not overengineer it with SRP.
> Or could it be another race that your lock is preventing?
Who knows, the race in fdexpand() looked quite obvious. And I did
not see another suspicious free.
> What about the diff below? Cheeky does it help?
OK bluhm@
>
> Index: kern/kern_descrip.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/kern_descrip.c,v
> retrieving revision 1.136
> diff -u -p -r1.136 kern_descrip.c
> --- kern/kern_descrip.c 24 Sep 2016 18:39:17 -0000 1.136
> +++ kern/kern_descrip.c 23 Jan 2017 10:40:53 -0000
> @@ -835,6 +835,16 @@ fdexpand(struct proc *p)
> nfiles = 2 * fdp->fd_nfiles;
>
> newofile = mallocarray(nfiles, OFILESIZE, M_FILEDESC, M_WAITOK);
> + /*
> + * Allocate all required chunks before calling free(9) to make
> + * sure that ``fd_ofiles'' stays valid if we go to sleep.
> + */
> + if (NDHISLOTS(nfiles) > NDHISLOTS(fdp->fd_nfiles)) {
> + newhimap = mallocarray(NDHISLOTS(nfiles), sizeof(u_int),
> + M_FILEDESC, M_WAITOK);
> + newlomap = mallocarray(NDLOSLOTS(nfiles), sizeof(u_int),
> + M_FILEDESC, M_WAITOK);
> + }
> newofileflags = (char *) &newofile[nfiles];
>
> /*
> @@ -853,11 +863,6 @@ fdexpand(struct proc *p)
> free(fdp->fd_ofiles, M_FILEDESC, fdp->fd_nfiles * OFILESIZE);
>
> if (NDHISLOTS(nfiles) > NDHISLOTS(fdp->fd_nfiles)) {
> - newhimap = mallocarray(NDHISLOTS(nfiles), sizeof(u_int),
> - M_FILEDESC, M_WAITOK);
> - newlomap = mallocarray(NDLOSLOTS(nfiles), sizeof(u_int),
> - M_FILEDESC, M_WAITOK);
> -
> copylen = NDHISLOTS(fdp->fd_nfiles) * sizeof(u_int);
> memcpy(newhimap, fdp->fd_himap, copylen);
> memset((char *)newhimap + copylen, 0,
