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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20111008103503.GA2148@netboy.at.omicron.at>
Date:	Sat, 8 Oct 2011 12:35:05 +0200
From:	Richard Cochran <richardcochran@...il.com>
To:	Eric Dumazet <eric.dumazet@...il.com>
Cc:	Johannes Berg <johannes@...solutions.net>,
	David Miller <davem@...emloft.net>, netdev@...r.kernel.org
Subject: Re: [RFC] net: remove erroneous sk null assignment in timestamping

On Sat, Oct 08, 2011 at 10:57:18AM +0200, Eric Dumazet wrote:
> Le samedi 08 octobre 2011 à 10:16 +0200, Johannes Berg a écrit :
> 
> > I'm not terribly familiar with struct sock. Looking at it, I'm a bit
> > confused by skb_orphan() -- it doesn't put the sock reference. So are
> > sockets not refcounted for skbs in this way? They seem to use
> > sock_wfree() which does a bit more than this it seems, and I don't see
> > it using sk_refcnt anywhere so I'm a bit confused now.

Me, too.

> Check following commit changelog to get some information on this.

Thanks, Eric, that does help explain.

>     We use this sock_hold()/sock_put() so that sock freeing
>     is delayed until all tx packets are completed.
>     
>     As we also update sk_wmem_alloc, we could offset sk_wmem_alloc
>     by one unit at init time, until sk_free() is called.
>     Once sk_free() is called, we atomic_dec_and_test(sk_wmem_alloc)
>     to decrement initial offset and atomicaly check if any packets
>     are in flight.
>     
>     skb_set_owner_w() doesnt call sock_hold() anymore

So, if I understand, then I can solve my particular problem by doing:

* skb_clone_tx_timestamp
	clone = skb_clone(skb, GFP_ATOMIC);
	skb_set_owner_w(clone, sk)
// instead of
//	clone->sk = sk;
	phydev->drv->txtstamp(phydev, clone, type);

* skb_complete_tx_timestamp
	serr->ee.ee_errno = ENOMSG;
	serr->ee.ee_origin = SO_EE_ORIGIN_TIMESTAMPING;
// just remove this:
//	skb->sk = NULL;
	err = sock_queue_err_skb(sk, skb);

The only problem(?) I see is that it violates the rules from sock.h,
quoted below. The cloned tx skb destined for the error queue would be
budgeted to sk_wmem_alloc while wait for the time stamp. But maybe we
can allow this?

from sock.h:
/*
 * Socket reference counting postulates.
 *
 * * Each user of socket SHOULD hold a reference count.
 * * Each access point to socket (an hash table bucket, reference from a list,
 *   running timer, skb in flight MUST hold a reference count.
                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                   /
BTW, no longer true.

 * * Packets, delivered from outside (from network or from another process)
 *   and enqueued on receive/error queues SHOULD NOT grab reference count,
 *   when they sit in queue. ~~~~~~~~~~~~~~~~~~~~~~~

Want to break/bend this rule.

Thanks,
Richard
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html