Re: [PATCH v5 06/10] Uprobes: Support SDT markers having reference count (semaphore)

[Ravi Bangoria]

Hi Oleg,

On 07/03/2018 11:53 PM, Oleg Nesterov wrote:
> forgot to mention...
> 
> Of course, I am not sure that UPROBE_KERN_CTR can actually work, there are a lot
> of details. But if it can, then we can also make ->ref_ctr_offset a consumer property.
> 
> On 07/03, Oleg Nesterov wrote:
>>
>> On 07/03, Ravi Bangoria wrote:
>>>
>>>> OK, and how exactly they update the counter? I mean, can we assume that, say,
>>>> bcc or systemtap can only increment or decrement it?
>>>
>>> I don't think we can assume anything here because this is all in user's
>>> control. User can even manually go and update the counter by directly
>>> hooking into the memory.
>>
>> Then how this all can work? I understand that user-space can do anything with
>> this counter, but we do not care if it does something wrong, say nullifies the
>> ctr incremented by kernel.
>>
>> I don't understand this. I think that if a user registers uprobe with
>> ->ref_ctr_offset != 0 we can safely assume that this is a counter, and we do
>> not care if userspace corrupts it.


Right, that's what I meant. We can't assume anything. Our(kernel) job
is just to:
 1. Increment / decrement counter in sync. No matter what is the value
    of counter before operation.
 2. Make sure that counter does not go negative by the kernel.
 3. Make sure that kernel is not harmed in any case.


>>
>>>> If yes, perhaps we can simplify the kernel code...
>>>
>>> Sure, let me know if you have any better idea.
>>
>> Can't we (ab)use the most significant bit in this counter?
>>
>> To simplify, lets suppose for the moment that 2 different uprobes can't have
>> the same ->ref_ctr_offset. Then we can do something like
>>
>> 	#define UPROBE_KERN_CTR		(SHRT_MAX + 1)	// MSB
>>
>> 	install_breakpoint:
>>
>> 		for (each valid_ref_ctr_vma which maps uprobe->ref_ctr_offset)
>> 			*ctr_ptr |= UPROBE_KERN_CTR;
>>
>> 		set_swbp();
>>
>> and
>>
>> 	remove_breakpoint:
>>
>> 		for (each valid_ref_ctr_vma which maps uprobe->ref_ctr_offset)
>> 			*ctr_ptr &= ~UPROBE_KERN_CTR;
>>
>> 		set_orig_insn();
>>
>> IOW, we increment/decrement by UPROBE_KERN_CTR, not by 1. But this way the
>> "increment" is idempotent, we do not care if "|=" or "&=" was applied more than
>> once, we do not need to record the fact that the counter was already incremented,
>> and inc/dec are always balanced.
>>
>>
>> Now, lets recall that multiple uprobes can share the same counter. install_breakpoint()
>> is still fine, and we only need to add the additional code into remove_breakpoint:
>>
>> 		for (each uprobe with the same inode and ref_ctr_offset)
>> 			if (filter_chain(uprobe))
>> 				goto keep_ctr;
>>
>> 		for (each valid_ref_ctr_vma which maps uprobe->ref_ctr_offset)
>> 			*ctr_ptr &= ~UPROBE_KERN_CTR;
>>
>> 	keep_ctr:
>> 		set_orig_insn();
>>
>>
>> Just an idea.
>>
>> What do you think?


This is really a cool idea. Let me explore that.

Thanks,
Ravi