On 16/04/15(Thu) 15:24, [email protected] 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?
I can't say if the actual behavior is correct or not, what do you think?
I believe all these cases must be carefully fixed first.
> Should I checkout CURRENT and patch it or 5.7 is fine too?
-current is where development happens :)
> I attach already exitig patches for git tree. If it required, I'll
> remake them and send one after another.
Don't flood us ;) But the next time, one diff *inline* with a clear
subject message is the preferred way so we can simply use our mailbox
to do peer review.
Regarding your previous question about the first diff I sent you:
> I don't understand, why we need to check flags inside fd_getfile
You don't *need* to, but doing so will remove a lot of gotos the
functions calling fd_getfile(). I'm aware it's not strictly necessary
but compare (without diff):
if ((fp = fd_getfile()) == NULL)
return (EBADF);
if (fp->f_flags & mode) == 0)
goto unref;
....
unref:
FRELE(fp)
to (with diff):
if ((fp = fd_getfile(mode)) == NULL)
return (EBADF);
Of course all the functions do not return EBADF when the mode is
incorrect, but I think the pattern is spread enough for this change
to be worth it.
