Re: [RFC] ndo_validate_skb: Let the netdev check a valid skb content

[Oliver Hartkopp <oliver@hartkopp.net>]

David Miller wrote:
> From: Oliver Hartkopp <oliver@hartkopp.net>
> Date: Wed, 23 Dec 2009 20:23:27 +0100
> 
>> @@ -2034,6 +2035,14 @@ int dev_queue_xmit(struct sk_buff *skb)
>>  			goto out_kfree_skb;
>>  	}
>>  
>> +	/* If the device offers a function to validate the skb content, let
>> +	 * it check the skb and return an error to the caller if it fails.
>> +	 */ 
>> +	if (ops->ndo_validate_skb && ops->ndo_validate_skb(skb)) {
>> +		rc = -EINVAL;
>> +		goto out_kfree_skb;
>> +	}
>> +
> 
> To me this is as valuable as no error at all and simply having
> the driver drop the frame.  Which is what we do now.  I can
> sniff at the receiver to see that the device never transmitted
> the frame.
> 
> Is this getting this generic -EINVAL back so important that it's
> worth adding the new if() test for the NULL method to every packet
> that traverses the machine?  Keep in mind we can route at rates
> exceeding 1 million packets per second, so ever cycle you add
> here really matters.
> 
> If you want to place the hooks still, put them out of the fast
> path, which is probably in AF_PACKET.  We can extend this to
> call the validation function from other "untrusted" sources.
> 
> But do realize that you're not saving anything, things like
> queueing disciplines and traffic classifiers in the qdisc
> layer can modify any part of the packet however they wish.
> 
> So even if the CAN protocol layer itself emitted a valid
> frame, the qdisc layer can "corrupt" it.

I see. Thanks for your detailed answer!

Indeed silently drop the invalid CAN frame inside the driver (and increase
dev->stats.tx_dropped) looks like the best solution to me now.

If people try to push rubbish into the device, tx_dropped is an appropriate
indication.

Thanks,
Oliver