lkml.org 
[lkml]   [2015]   [Nov]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH, RESEND] ipc/shm: handle removed segments gracefully in shm_mmap()
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


\
 
 \ /
  Last update: 2015-11-13 10:41    [W:0.084 / U:0.036 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site