Re: [PATCHv3 05/16] pps: access pps device by direct pointer

[Alexander Gordeev]

Ð Thu, 5 Aug 2010 11:32:36 +0200
Rodolfo Giometti <giometti@xxxxxxxxxxxx> ÐÐÑÐÑ:

> On Thu, Aug 05, 2010 at 01:06:42AM +0400, Alexander Gordeev wrote:
> > Using device index as a pointer needs some unnecessary work to be done
> > every time the pointer is needed (in irq handler for example).
> > Using a direct pointer is much more easy (and safe as well).
> > 
> > Signed-off-by: Alexander Gordeev <lasaine@xxxxxxxxxxxxx>
[snip]
> 
> If you remove these functions you can't be sure anymore that nobodies
> may call pps_event() over a non existent device...

[snip]

> By dropping pps_get_source you may be here by a call from (i.e.) a
> serial port driver whose doesn't know if your PPS source is gone or
> not...
> 
> I don't understand how your modifications may resolve this problem.

Well, this can happen only if PPS client module calls pps_event before
calling pps_register_source() or after pps_unregister_source(). This
means that it's broken! If we try to handle/workaround broken clients it
affects performance. So we have to choose what is the priority:
security or performance. My guru told me I shouldn't bother too much
about broken kernel-space code which my code interacts with. If it's
broken it should be fixed. Some assertions enabled by DEBUG define are
enough. For me it makes sense but I don't know what should I check here?

Well I can add something like that to pps_event:

BUG_ON((pps == NULL) || (pps_get_source(pps->id) != pps));

where pps_get_source is:

static inline struct pps_device *pps_get_source(int source)
{
	struct pps_device *pps;
	unsigned long flags;

	spin_lock_irqsave(&pps_idr_lock, flags);
	pps = idr_find(&pps_idr, source);
	spin_unlock_irqrestore(&pps_idr_lock, flags);

	return pps;
}

BTW, while looking at the code to answer your question I've found a
bug: struct pps_device was not kfree'd on device destruction. The fix
will appear soon. Perhaps with an assertion above if you like it.

-- 
  Alexander

Attachment: signature.asc
Description: PGP signature