lkml.org 
[lkml]   [2015]   [Nov]   [16]   [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 Fri, Nov 13, 2015 at 11:23:10AM -0800, Davidlohr Bueso wrote:
> On Fri, 13 Nov 2015, Kirill A. Shutemov wrote:
>
> >On Thu, Nov 12, 2015 at 09:31:37PM -0800, Davidlohr Bueso wrote:
> >>On Wed, 11 Nov 2015, Kirill A. Shutemov wrote:
> >>>>> 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.
>
> I was referring to the fact that I hate having to prematurely call shm_open()
> just for this case, and then have to backout, ie for nattach. Similarly, I
> dislike that you make shm_close behave one way and _shm_open another, looks
> hacky.
>
> That said, I do agree that we should inform EIDRM back to the shm_mmap
> caller. My immediate thought would be to recheck right after shm_open returns.
> I realize this is also hacky as we run into similar inconsistencies that I
> mentioned above. But that's a caller (and the only one), not the whole
> shm_open/close. Also, just like we are concerned about EIDRM, should we also
> care about EINVAL -- where we race with explicit user shmctl(RMID) calls but
> we hold reference to nattach?? I mean, why bother doing mmap if the segment is
> marked for deletion and ipc won't touch it again anyway (failed idr lookups).
> The downside to that is the extra lookup overhead, so perhaps your approach
> is better. But looks like the right thing to do conceptually. Something like so?
>
> shm_mmap()
> {
> err = shm_check_vma_validity()
> if (err)
>
> ->mmap()
>
> shm_open()
> err = shm_check_vma_validity()
> if (err)
> return err; /* shm_open was a nop, return the corresponding error */
>
> return 0;
> }

The problem I have with this approach is that it assumes that there's
nothing to undo from ->mmap in case of shm_check_validity() failed in the
second call. That seems true at the moment, but I'm not sure if we can
assume this in general and if it's future-proof.

> So considering EINVAL, even your approach to bumping up nattach by calling
> _shm_open earlier isn't enough. Races exposed to user called rmid can still
> occur between dropping the lock and doing ->mmap().

Ugh.. I see. That's a problem.

Looks like a problem we solved for mm_struct by separation of mm_count
from mm_users. Should we have two counters instead of shm_nattch?

> Ultimately this leads to all ipc_valid_object() checks, as we totally
> ignore SHM_DEST segments nowadays since we forbid mapping previously
> removed segments.
>
> I think this is the first thing we must decide before going forward with this
> mess. ipc currently defines invalid objects by merely checking the deleted flag.

To me all these flags mess should be replaced by proper refcounting.
Although, I admit, I don't understand SysV IPC API good enough to say for
sure if it's possible.

--
Kirill A. Shutemov


\
 
 \ /
  Last update: 2015-11-16 11:01    [W:0.092 / U:0.744 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site