Re: [RFC] net: remove erroneous sk null assignment in timestamping

[Richard Cochran <richardcochran@gmail.com>]

On Fri, Oct 07, 2011 at 01:33:56PM -0400, David Miller wrote:
> It looks like skb_clone_tx_timestamp() sets clone->sk without any
> proper refcounting, so I bet this NULL'ing it out is working
> around that bug.

I don't remember why I put it that way, but I took a look at the
problem, and I am not sure how to solve it. The other callers of
sock_queue_err_skb all create or clone the error skb immediately
before queueing it:

  net/core/skbuff.c:       skb_tstamp_tx
  net/ipv4/ip_sockglue.c:  ip_icmp_error, ip_local_error
  net/ipv6/datagram.c:     ipv6_icmp_error, ipv6_local_error

So I need to prevent the socket from disappearing between
skb_clone_tx_timestamp and skb_complete_tx_timestamp:

  skb_clone_tx_timestamp
	clone = skb_clone(skb, GFP_ATOMIC);
	sock_hold
  skb_complete_tx_timestamp
	sock_queue_err_skb(sk, skb);
	sock_put

What do you think?

BTW, while looking for a good pattern to follow, I found that the can
driver also sets skb->sk after clone with no special treatment, like
so:

  drivers/net/can/dev.c:285
	can_put_echo_skb
		struct sock *srcsk = skb->sk;
		skb = skb_clone(old_skb, GFP_ATOMIC);
		skb->sk = srcsk;

> The TX side of this infrastructure seems very poorly tested.

In fact, we do have the phyter driver used in an extensive automated
test farm, but the applications just don't do the kinds of things
suggested to trigger the problem. The normal pattern is, send event
packet, get tx timestamp, and so we haven't seen the bug at all.

Thanks,
Richard