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,
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...
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.
o My shm_check_vma_validity() also deals with IPC_RMID as we do the
ipc_valid_object() check.
Mine too:
shm_mmap()
__shm_open()
shm_lock()
ipc_lock()
ipc_valid_object()
Or I miss something?
Sorry, I meant ipc_obtain_object_idr, so EINVAL is also accounted for, we
the segment is already deleted and not only marked as such.
o We have a new WARN where necessary, instead of having one now is shm_open.
I'm not sure why you think that shm_close() which was never paired with
successful shm_open() doesn't deserve WARN().
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...
> 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've got impression that I miss something important about how locking in
IPC/SHM works, but I cannot grasp what.. Hm?.
Could you be more specific? The only lock involved here is the ipc object lock,
if you haven't, you might want to refer to ipc/util.c which has a brief ipc
locking description.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>