Re: [PATCH 19/21] i386 Kprobes semaphore fix

[Prasanna S Panchamukhi <prasanna@in.ibm.com>]

resending this mail, since my earlier email did not reach lkml.
On Wed, Nov 09, 2005 at 03:07:55PM +0530, Prasanna S Panchamukhi wrote:
> Zach,
> 
> Thanks for doing this.
> 
> On Tue, Nov 08, 2005 at 05:36:53AM -0800, Zachary Amsden wrote:
> > Andi Kleen wrote:
> > 
> > >On Tuesday 08 November 2005 05:39, Zachary Amsden wrote:
> > > 
> > >
> > >>IA-32 linear address translation is loads of fun.
> > >>   
> > >>
> > >
> > >Thanks for doing that audit work. Can you please double check x86-64 code 
> > >is
> > >ok? 
> > >
> > >Actually giving all that complexity maybe it would be better to just
> > >stop handling the case and remove all that. I'm not sure what kprobes 
> > >needs it for - it doesn't even handle user space yet and even if it ever 
> > >does it is unlikely that handling 16bit code makes much sense. And the 
> 
> 
> The code was added to address the problem related to stealing of interrupts from
> VM86. Please see the discussion thread for more details from the URL below
> http://lkml.org/lkml/2004/11/9/214
> 
> > But were kprobes even inteneded for userspace?  There are races here 
> > that are difficult to close without some heavy machinery, and I would 
> > rather not put the machinery in place if simplifying the code is the 
> > right answer.
> 
> Presently kprobes supports only kernel space probes. Work is in progress
> for user space probes support.
> 
> >+       addr = (kprobe_opcode_t *)convert_eip_to_linear(regs,
> >+                                       regs->eip -
> >sizeof(kprobe_opcode_t),
> >+                                       &current->mm->context, &limit);
> >+
> 
> Instead you can check if it is in kernel mode and calculate the address directly 
> first, since it is in the fast path.
> 		addr = regs->eip - sizeof(kprobe_opcode_t);
> 	else
> 		addr = convert_eip_to_linear(..);
> 
> there by avoiding calling convert_eip_to_linear () in case of every kernel probes.
> 
> 
> >+       /* Don't let userspace races re-address into kernel space */
> >+       if ((unsigned long)addr > limit)
> >+               return 0;
> 
> there is no need for this check here in the fast path, because kprobes handles this 
> case by checking if the address is on the kprobes hash list and later returning 
> from that point.
> 
> Please make sure it pass the test case discussed in the thread, URL is below.
> http://lkml.org/lkml/2004/11/9/214
> 
> Thanks
> -Prasanna
> --
> Prasanna S Panchamukhi
> Linux Technology Center
> India Software Labs, IBM Bangalore
> Ph: 91-80-25044636
> <prasanna@in.ibm.com>

-- 
Have a Nice Day!

Thanks & Regards
Prasanna S Panchamukhi
Linux Technology Center
India Software Labs, IBM Bangalore
Ph: 91-80-25044636
<prasanna@in.ibm.com>