On 17 Apr 2015, at 12:49, Martin Pieuchot wrote:
> On 16/04/15(Thu) 15:24, kanonenvogel@gmail.com wrote:
>> Well, lets begin.
>>
>> In the future, I wish to have fd_getfile() returning acquired fp instance.
>> The goal is to not to have pointer to destroyed fp instance
>> in FREF()/FRELE()/fd_getfile() races. This one requres modification of
>> getsock(), getvnode() and dupfdopen() functions, they must receive pointer to
>> struct proc instance for FRELE() call on referenced fp instance while they
>> have internal error cases. While getsock(), getvnode() and dupfdopen()
>> functions are called, "struct proc" instance exists, so their
>> "struct filedesc *" arg can be replaced by "struct proc *" arg which contains
>> pointer to "struct filedesc”.
>>
>> The races will be appeared right after at least one FRELE(), FREF() or
>> fd_getfile() call will be done outside kernel lock. The “outside kernel
>> lock" call
>> capability requires a little more refactoring, but for this functions only,
>> not
>> system-wide.
>>
>> Now we have something like:
>>
>> if((fp = fd_getfile(fds, fd)) == NULL)
>> goto error;
>>
>> /*
>> * fp can be destroyed here by FRELE() call on other cpu
>> */
>>
>> FREF(fp);
>>
>> The goal is to avoid this situation.
>
> I came to the same conclusion when I wrote these diffs. I think the
> first 3 are good and simple enough to be committed, somebody else agree?
>
> The fourth one that move FREF() inside fd_getfile() is IMHO incomplete.
> As you can see I putted some XXX where the existing code was calling
> fd_getfile() without incrementing the reference count. Why did you
> decide to delete them?
Well, my version of fd_getfile() requires “struct proc *” arg for proper
funref() call within. So my original patchset has my own modification of
getsock(), getvnode() and dupfdopen(). They has “struct filedesc *” and
“struct file *” arguments together, but the only “struct file *" is enough.
And I used your version. :)
At first i modified fd_getfile() internals for acquisition/release races
on smp machine. Atomic increment of f_count is required in smp case.
Kernel sources has the strange and wrong magic with f_count field. closef()
wants fp->f_count to be >= 2, and closef()’s callers do this magic for panic
prevention. falloc() and FILE_SET_MATURE() do this magic too. Now,
newly created “struct file” instance has f_count == 2. It’s wrong, and below I
describe situation without this magic.
Minimal f_count value of existing “struct file” instance is 1.
Minimal f_count value of existing and acquired “struct file” instance is 2.
Minimal f_count value of acquired “struct file” instance is 1.
“existing” means “exists in “struct filedesc” instance and can be obtained by
fd_getfile() call”.
“acquired” means “returned by fd_getfile() call”.
“struct filedesc” instance holds existing “struct file” instances.
fdalloc() creates and places “struct file” instance to “struct filedesc”
container
fdrelease() removes “struct file” instance from “struct filedesc”, but delete it
only if f_count is 1 (fp exists and has no references). fp instance can be
shared between multiple processes and can be shared by multiple
“struct file” correlated syscalls of one multithreaded process. So, we have
races and situation can be: “fp instance” must be removed from
“struct filedesc” but kept for current “struct file” instance users. Last of
them
will destroy it by FRELE() call. If we don’t increment f_count within
fd_getfile(),
we can return pointer to already destroyed “struct file” instance. For example
one thread calls fd_getfile() and other calls fdrelease(). If fd_getfile()
caller
wins, f_count is at least 1 and instance is not destroyed, but removed from
“struct filedesc” container and can’t be obtained by next fd_getfile() call. if
fdrelease() wins it removes and destroys “struct file” instance and fd_getfile()
will return NULL.
the real fucntions must be:
struct file *
fd_getfile(struct filedesc *fdp, int fd, struct proc *p)
{
#ifdef MULTIPROCESSOR
struct file *fp;
unsigned long count;
if(fd <0 || fd >= fdp->fd_nfiles)
return NULL;
restart:
if ((fp = fdp->fd_ofiles[fd]) == NULL)
return NULL;
if ((count = fp->f_count) == 0)
return NULL;
if (atomic_cas_ulong(&fp->f_count, count, count + 1) != count)
goto restart;
if ((fp != fdp->fd_ofiles[fd])) {
funref(fp, p);
goto restart;
}
if (!FILE_IS_USABLE(fp)) {
funref(fp, p);
return (NULL);
}
return fp;
#else
struct file *fp;
if(fd <0 || fd >= fdp->fd_nfiles)
return NULL;
if ((fp != fdp->fd_ofiles[fd]))
return NULL;
if (!FILE_IS_USABLE(fp))
return NULL;
fp->f_count++;
return fp;
#endif
}
/*
* FREF() replacement, must be called on
*
