| Message ID | 20121128223300.GA12527@shrek.podlesie.net |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
On Wed, 2012-11-28 at 23:33 +0100, Krzysztof Mazur wrote: > > 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 <krzysiek@podlesie.net> 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! In <20121006154606.GA25588@shrek.podlesie.net> you posted another patch: > 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...?
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: > > > > 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 <krzysiek@podlesie.net> > > 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 <20121006154606.GA25588@shrek.podlesie.net> you posted another patch: > > 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/linux/atmdev.h b/include/linux/atmdev.h index 57bd93f..648fb79 100644 --- a/include/linux/atmdev.h +++ b/include/linux/atmdev.h @@ -291,6 +291,14 @@ void vcc_release_async(struct atm_vcc *vcc, int reply); */ void vcc_pop(struct atm_vcc *vcc, struct sk_buff *skb); +/** + * vcc_pop_skb - free transmitted ATM skb + * + * This variant of vcc_pop() assumes that ATM_SKB(skb)->vcc is set + * by driver. + */ +void vcc_pop_skb(struct sk_buff *skb); + struct atm_ioctl { struct module *owner; /* A module reference is kept if appropriate over this call. diff --git a/net/atm/common.c b/net/atm/common.c index c42ff62..378c911 100644 --- a/net/atm/common.c +++ b/net/atm/common.c @@ -663,6 +663,12 @@ void vcc_pop(struct atm_vcc *vcc, struct sk_buff *skb) } EXPORT_SYMBOL(vcc_pop); +void vcc_pop_skb(struct sk_buff *skb) +{ + vcc_pop(ATM_SKB(skb)->vcc, skb); +} +EXPORT_SYMBOL(vcc_pop_skb); + unsigned int vcc_poll(struct file *file, struct socket *sock, poll_table *wait) { struct sock *sk = sock->sk;