Re: [PATCH v7 3.2-rc2 4/30] uprobes: Define hooks for mmap/munmap.

[Srikar Dronamraju]

> > The rules that I am using are: 
> > 
> > mmap_uprobe() increments the count if 
> >         - it successfully adds a breakpoint.
> >         - it not add a breakpoint, but sees that there is a underlying
> >           breakpoint (via a read_opcode call).
> > 
> > munmap_uprobe() decrements the count if 
> >         - it sees a underlying breakpoint,  (via  a read_opcode call)
> >         - Subsequent unregister_uprobe wouldnt find the breakpoint
> >           unless a mmap_uprobe kicks in, since the old vma would be
> >           dropped just after munmap_uprobe.
> > 
> > register_uprobe increments the count if:
> >         - it successfully adds a breakpoint.
> > 
> > unregister_uprobe decrements the count if:
> >         - it sees a underlying breakpoint and removes successfully. 
> >                         (via a read_opcode call)
> >         - Subsequent munmap_uprobe wouldnt find the breakpoint
> >           since there is no underlying breakpoint after the
> >           breakpoint removal. 
> 
> The problem I'm having is that such stuff isn't included in the patch
> set.
> 
> We've got both comments in the C language and Changelog in our patch
> system, yet you consistently fail to use either to convey useful
> information on non-trivial bits like this.
> 

Agree, I will put this as part of comments.

> This leaves the reviewer wondering if you've actually considered stuff
> properly, then me actually finding bugs in there does of course
> undermine that even further.
> 
> What I really would like is for this patch set not to have such subtle
> stuff at all, esp. at first. Once its in and its been used a bit we can
> start optimizing and add subtle crap like this.

We actually started the discussion of why we increment the count in
mmap_uprobe() in EEXIST case (and read_opcode()). It exists for two
reasons.
	- To handle fork case (that I wrote in another mail).
	- To handle mremap.(the case where we are discussing now)

I would contend that removing the breakpoint in munmap doesnt amount to
optimization. Since the start of unmap(), there cannot be another
remove_breakpoint called for the vma,vaddr tuple, until the vma is
cleaned up, or the subsequent mmap() is done. So the case of accounting
for an already decremented count should never occur.

I was following the general convention being used within the kernel to not
bother about the area that we are going to unmap. For example: If a ptraced
area were to be unmapped or remapped, I dont see the breakpoint being
removed and added back. Also if a ptrace process is exitting, we dont go
about removing the installed breakpoints.

Also we would still need the check for EEXIST and read_opcode for handling
the fork() case. So even if we add extra line to remove the actual
breakpoint in munmap, It doesnt make the code any more simpler.

-- 
Thanks and Regards
Srikar

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/