On Thu, Nov 12, 2015 at 09:31:37PM -0800, Davidlohr Bueso wrote:
> On Wed, 11 Nov 2015, Kirill A. Shutemov wrote:
> >And I had concern about your approach:
> >
> > If I read it correctly, with the patch we would ignore locking
> > failure inside shm_open() and mmap will succeed in this case. So
> > the idea is to have shm_close() no-op and therefore symmetrical.
>
> Both open and close are no-ops in the case the segment has been removed,
The part I disagree is that shm_open() shouldn't be allowed for removed
segment. Basically, I prefer to keep the policy we have now.
> that's the symmetrical, and I'm not sure I follow -- we don't ignore locking
> failure in shm_open _at all_. Just like your approach, all I do is return if
> there's an error...
As you wrote in the comment, shm_check_vma_validity() check is racy. It's
just speculative check which doesn't guarantee that shm_lock() in
shm_open() will succeed. If this race happen, you just ignore this locking
failure and proceed. You compensate this, essentially failed shm_open(),
by no-op in shm_close().
In my opinion, failed shm_lock() in shm_open() should lead to returning
error from shm_mmap(). And there's no need in shm_close() hackery.
My patch tries to implement this.
>
> > That's look fragile to me. We would silently miss some other
> > broken open/close pattern.
>
> Such cases, if any, should be fixed and handled appropriately, not hide
> it under the rung, methinks.
But, don't you think you *do* hide such cases? With you patch pattern like
shm_open()-shm_close()-shm_close() will not trigger any visible effect.
> >>o My no-ops explicitly pair.
> >
> >As I said before, I don't think we should ignore locking error in
> >shm_open(). If we propagate the error back to caller shm_close() should
> >never happen, therefore no-op is unneeded in shm_close(): my patch trigger
> >WARN() there.
>
> Yes, you WARN() in shm_close, but you still make it a no-op...
We can crash kernel with BUG_ON() there, but would it help anyone?
The WARN() is just to make broken open/close visible.
> >>> ret = sfd->file->f_op->mmap(sfd->file, vma);
> >>>- if (ret != 0)
> >>>+ if (ret) {
> >>>+ shm_close(vma);
> >>> return ret;
> >>>+ }
> >>
> >>Hmm what's this shm_close() about?
> >
> >Undo shp->shm_nattch++ in successful __shm_open().
>
> Yeah that's just nasty.
I don't see why: we successfully opened the segment, but f_op->mmap
failed -- let's close the segment. It's normal error path.
--
Kirill A. Shutemov
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
