Re: [PATCH] atm: introduce vcc_pop_skb()

On Mon, Dec 03, 2012 at 01:22:41PM +0000, David Woodhouse wrote:

On Wed, 2012-11-28 at 23:33 +0100, Krzysztof Mazur wrote:

quoted

Many ATM drivers store vcc in ATM_SKB(skb)->vcc and use it for
freeing skbs. Now they can just use atm_pop_skb() to free such
buffers.

Signed-off-by: Krzysztof Mazur <redacted>

Note that this one didn't make it into the tree that Dave just pulled.

Not that I didn't think it was a good idea, but it was just separate
from the other "real" fixes ??? and the tree had already grown into a big
enough pile from your original single patch!

That patch is a preparation of separate series. The current version
(far from final version) is available at:

git://git.podlesie.net/km/linux.git atm-pop

and

http://git.podlesie.net/gitweb.cgi?p=km/linux.git;a=shortlog;h=refs/heads/atm-pop

Patch 3 and especially patch 4 are far from being ready. They are also ugly
because many ATM drivers use strange coding style and I tried to use that
style because using different style for new code would be probably be even worse.

Currently there are 4 patches:
      atm: introduce vcc_pop()
      atm: introduce vcc_pop_skb()
      atm: convert drivers to use vcc_pop*()
      atm: add missing vcc_pop*() calls in drivers

The first two introduce two helpers vcc_pop() and vcc_pop_skb(). The third
should be 1:1 conversion of vcc->pop() users to vcc_pop*() interface.
The forth patch fixes some problems I've found. In all cases the bugs
occurs in error handling code, in most cases dev_kfree_skb() is used
instead of vcc_pop(), in some cases driver just returns some error
code and skb is never freed, in two cases I removed the vcc->pop()
call in code like:

static int eni_send(struct atm_vcc *vcc,struct sk_buff *skb)
{
	[...]
	if (!skb) {
		printk(KERN_CRIT "!skb in eni_send ?\n");
		if (vcc->pop) vcc->pop(vcc,skb);
		return -EINVAL;
	}

I don't think that we should check for !skb and even if skb == NULL
it's not a good idea to call vcc->pop() because it will crash.

Current diffstat:
 drivers/atm/adummy.c     |  5 +---
 drivers/atm/ambassador.c | 34 ++++++++++++++--------------
 drivers/atm/atmtcp.c     | 15 ++++--------
 drivers/atm/eni.c        | 11 ++++-----
 drivers/atm/firestream.c | 19 ++--------------
 drivers/atm/fore200e.c   | 23 ++++---------------
 drivers/atm/he.c         | 33 ++++++---------------------
 drivers/atm/horizon.c    | 31 +++++++++----------------
 drivers/atm/idt77252.c   | 32 +++++++-------------------
 drivers/atm/iphase.c     | 59 +++++++++++++-----------------------------------
 drivers/atm/lanai.c      | 18 ++++-----------
 drivers/atm/nicstar.c    | 31 ++++++++-----------------
 drivers/atm/solos-pci.c  |  5 +---
 drivers/atm/zatm.c       | 13 ++++-------
 drivers/usb/atm/usbatm.c | 17 ++++----------
 include/linux/atmdev.h   | 16 +++++++++++++
 net/atm/common.c         | 15 ++++++++++++
 17 files changed, 128 insertions(+), 249 deletions(-)

In [ref] you posted another patch:

quoted

I think there is another problem here. The pppoatm gets a reference
to atmvcc, but I don't see anything that protects against removal
of that vcc.

The vcc uses vcc->sk socket for reference counting, so sock_hold()
and sock_put() should be used by pppoatm.

That one I think *isn't* needed, because we have properly fixed the
races with vcc_destroy_socket(). I just wanted to check you agree...?

It was never really needed, I removed it from v3.

Thanks,

Krzysiek