[PATCH 00/17] ATM fixes for pppoatm/br2684

[PATCH 00/17] ATM fixes for pppoatm/br2684

[David Woodhouse]

This is the result of pulling on the thread started by Krzysztof Mazur's
original patch 'pppoatm: don't send frames to destroyed vcc'.

Various problems in the pppoatm and br2684 code are solved, some of which
were easily triggered and would panic the kernel.

The patch series can be pulled from
	git://git.infradead.org/users/dwmw2/atm.git
or viewed at 
	http://git.infradead.org/users/dwmw2/atm.git

DaveM, please wait for an ack from Krzysztof and Chas before pulling this.

David Woodhouse (9):
      solos-pci: Wait for pending TX to complete when releasing vcc
      br2684: don't send frames on not-ready vcc
      atm: Add release_cb() callback to vcc
      pppoatm: fix missing wakeup in pppoatm_send()
      br2684: fix module_put() race
      pppoatm: optimise PPP channel wakeups after sock_owned_by_user()
      solos-pci: clean up pclose() function
      solos-pci: use GFP_KERNEL where possible, not GFP_ATOMIC
      solos-pci: remove list_vccs() debugging function

Krzysztof Mazur (7):
      atm: add owner of push() callback to atmvcc
      pppoatm: allow assign only on a connected socket
      pppoatm: fix module_put() race
      pppoatm: take ATM socket lock in pppoatm_send()
      pppoatm: drop frames to not-ready vcc
      pppoatm: do not inline pppoatm_may_send()
      br2684: allow assign only on a connected socket

Nathan Williams (1):
      solos-pci: Fix leak of skb received for unknown vcc

[PATCH 01/17] atm: add owner of push() callback to atmvcc

[David Woodhouse]

From: Krzysztof Mazur <krzysiek@podlesie.net>

The atm is using atmvcc->push(vcc, NULL) callback to notify protocol
that vcc will be closed and protocol must detach from it. This callback
is usually used by protocol to decrement module usage count by module_put(),
but it leaves small window then module is still used after module_put().

Now the owner of push() callback is kept in atmvcc and
module_put(atmvcc->owner) is called after the protocol is detached from vcc.

Signed-off-by: Krzysztof Mazur <krzysiek@podlesie.net>
Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
Acked-by: Chas Williams <chas@cmf.nrl.navy.mil>
---
 include/linux/atmdev.h | 1 +
 net/atm/common.c       | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/include/linux/atmdev.h b/include/linux/atmdev.h
index 22ef21c..72db2af 100644
--- a/include/linux/atmdev.h
+++ b/include/linux/atmdev.h
@@ -106,6 +106,7 @@ struct atm_vcc {
 	void		*dev_data;	/* per-device data */
 	void		*proto_data;	/* per-protocol data */
 	struct k_atm_aal_stats *stats;	/* pointer to AAL stats group */
+	struct module *owner;		/* owner of ->push function */
 	/* SVC part --- may move later ------------------------------------- */
 	short		itf;		/* interface number */
 	struct sockaddr_atmsvc local;
diff --git a/net/atm/common.c b/net/atm/common.c
index 0c0ad93..2421664 100644
--- a/net/atm/common.c
+++ b/net/atm/common.c
@@ -156,6 +156,7 @@ int vcc_create(struct net *net, struct socket *sock, int protocol, int family)
 	atomic_set(&sk->sk_rmem_alloc, 0);
 	vcc->push = NULL;
 	vcc->pop = NULL;
+	vcc->owner = NULL;
 	vcc->push_oam = NULL;
 	vcc->vpi = vcc->vci = 0; /* no VCI/VPI yet */
 	vcc->atm_options = vcc->aal_options = 0;
@@ -175,6 +176,7 @@ static void vcc_destroy_socket(struct sock *sk)
 			vcc->dev->ops->close(vcc);
 		if (vcc->push)
 			vcc->push(vcc, NULL); /* atmarpd has no push */
+		module_put(vcc->owner);
 
 		while ((skb = skb_dequeue(&sk->sk_receive_queue)) != NULL) {
 			atm_return(vcc, skb->truesize);
-- 
1.8.0

[PATCH 02/17] pppoatm: allow assign only on a connected socket

[David Woodhouse]

From: Krzysztof Mazur <krzysiek@podlesie.net>

The pppoatm does not check if used vcc is in connected state,
causing an Oops in pppoatm_send() when vcc->send() is called
on not fully connected socket.

Now pppoatm can be assigned only on connected sockets; otherwise
-EINVAL error is returned.

Signed-off-by: Krzysztof Mazur <krzysiek@podlesie.net>
Cc: Chas Williams - CONTRACTOR <chas@cmf.nrl.navy.mil>
Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
---
 net/atm/pppoatm.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/atm/pppoatm.c b/net/atm/pppoatm.c
index 226dca9..f27a07a 100644
--- a/net/atm/pppoatm.c
+++ b/net/atm/pppoatm.c
@@ -406,6 +406,8 @@ static int pppoatm_ioctl(struct socket *sock, unsigned int cmd,
 			return -ENOIOCTLCMD;
 		if (!capable(CAP_NET_ADMIN))
 			return -EPERM;
+		if (sock->state != SS_CONNECTED)
+			return -EINVAL;
 		return pppoatm_assign_vcc(atmvcc, argp);
 		}
 	case PPPIOCGCHAN:
-- 
1.8.0

[PATCH 03/17] pppoatm: fix module_put() race

[David Woodhouse]

From: Krzysztof Mazur <krzysiek@podlesie.net>

The pppoatm used module_put() during unassignment from vcc with
hope that we have BKL. This assumption is no longer true.

Now owner field in atmvcc is used to move this module_put()
to vcc_destroy_socket().

Signed-off-by: Krzysztof Mazur <krzysiek@podlesie.net>
Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
---
 net/atm/pppoatm.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/net/atm/pppoatm.c b/net/atm/pppoatm.c
index f27a07a..b23c672 100644
--- a/net/atm/pppoatm.c
+++ b/net/atm/pppoatm.c
@@ -60,6 +60,7 @@ struct pppoatm_vcc {
 	struct atm_vcc	*atmvcc;	/* VCC descriptor */
 	void (*old_push)(struct atm_vcc *, struct sk_buff *);
 	void (*old_pop)(struct atm_vcc *, struct sk_buff *);
+	struct module *old_owner;
 					/* keep old push/pop for detaching */
 	enum pppoatm_encaps encaps;
 	atomic_t inflight;
@@ -155,8 +156,6 @@ static void pppoatm_unassign_vcc(struct atm_vcc *atmvcc)
 	ppp_unregister_channel(&pvcc->chan);
 	atmvcc->user_back = NULL;
 	kfree(pvcc);
-	/* Gee, I hope we have the big kernel lock here... */
-	module_put(THIS_MODULE);
 }
 
 /* Called when an AAL5 PDU comes in */
@@ -165,9 +164,13 @@ static void pppoatm_push(struct atm_vcc *atmvcc, struct sk_buff *skb)
 	struct pppoatm_vcc *pvcc = atmvcc_to_pvcc(atmvcc);
 	pr_debug("\n");
 	if (skb == NULL) {			/* VCC was closed */
+		struct module *module;
+
 		pr_debug("removing ATMPPP VCC %p\n", pvcc);
+		module = pvcc->old_owner;
 		pppoatm_unassign_vcc(atmvcc);
 		atmvcc->push(atmvcc, NULL);	/* Pass along bad news */
+		module_put(module);
 		return;
 	}
 	atm_return(atmvcc, skb->truesize);
@@ -362,6 +365,7 @@ static int pppoatm_assign_vcc(struct atm_vcc *atmvcc, void __user *arg)
 	atomic_set(&pvcc->inflight, NONE_INFLIGHT);
 	pvcc->old_push = atmvcc->push;
 	pvcc->old_pop = atmvcc->pop;
+	pvcc->old_owner = atmvcc->owner;
 	pvcc->encaps = (enum pppoatm_encaps) be.encaps;
 	pvcc->chan.private = pvcc;
 	pvcc->chan.ops = &pppoatm_ops;
@@ -378,6 +382,7 @@ static int pppoatm_assign_vcc(struct atm_vcc *atmvcc, void __user *arg)
 	atmvcc->push = pppoatm_push;
 	atmvcc->pop = pppoatm_pop;
 	__module_get(THIS_MODULE);
+	atmvcc->owner = THIS_MODULE;
 
 	/* re-process everything received between connection setup and
 	   backend setup */
-- 
1.8.0

[PATCH 04/17] pppoatm: take ATM socket lock in pppoatm_send()

[David Woodhouse]

From: Krzysztof Mazur <krzysiek@podlesie.net>

The pppoatm_send() does not take any lock that will prevent concurrent
vcc_sendmsg(). This causes two problems:

	- there is no locking between checking the send queue size
	  with atm_may_send() and incrementing sk_wmem_alloc,
	  and the real queue size can be a little higher than sk_sndbuf

	- the vcc->sendmsg() can be called concurrently. I'm not sure
	  if it's allowed. Some drivers (eni, nicstar, ...) seem
	  to assume it will never happen.

Now pppoatm_send() takes ATM socket lock, the same that is used
in vcc_sendmsg() and other ATM socket functions. The pppoatm_send()
is called with BH disabled, so bh_lock_sock() is used instead
of lock_sock().

Signed-off-by: Krzysztof Mazur <krzysiek@podlesie.net>
Cc: Chas Williams - CONTRACTOR <chas@cmf.nrl.navy.mil>
Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
---
 net/atm/pppoatm.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/net/atm/pppoatm.c b/net/atm/pppoatm.c
index b23c672..c4a57bc 100644
--- a/net/atm/pppoatm.c
+++ b/net/atm/pppoatm.c
@@ -272,10 +272,19 @@ static inline int pppoatm_may_send(struct pppoatm_vcc *pvcc, int size)
 static int pppoatm_send(struct ppp_channel *chan, struct sk_buff *skb)
 {
 	struct pppoatm_vcc *pvcc = chan_to_pvcc(chan);
+	struct atm_vcc *vcc;
+	int ret;
+
 	ATM_SKB(skb)->vcc = pvcc->atmvcc;
 	pr_debug("(skb=0x%p, vcc=0x%p)\n", skb, pvcc->atmvcc);
 	if (skb->data[0] == '\0' && (pvcc->flags & SC_COMP_PROT))
 		(void) skb_pull(skb, 1);
+
+	vcc = ATM_SKB(skb)->vcc;
+	bh_lock_sock(sk_atm(vcc));
+	if (sock_owned_by_user(sk_atm(vcc)))
+		goto nospace;
+
 	switch (pvcc->encaps) {		/* LLC encapsulation needed */
 	case e_llc:
 		if (skb_headroom(skb) < LLC_LEN) {
@@ -288,8 +297,10 @@ static int pppoatm_send(struct ppp_channel *chan, struct sk_buff *skb)
 			}
 			consume_skb(skb);
 			skb = n;
-			if (skb == NULL)
+			if (skb == NULL) {
+				bh_unlock_sock(sk_atm(vcc));
 				return DROP_PACKET;
+			}
 		} else if (!pppoatm_may_send(pvcc, skb->truesize))
 			goto nospace;
 		memcpy(skb_push(skb, LLC_LEN), pppllc, LLC_LEN);
@@ -299,6 +310,7 @@ static int pppoatm_send(struct ppp_channel *chan, struct sk_buff *skb)
 			goto nospace;
 		break;
 	case e_autodetect:
+		bh_unlock_sock(sk_atm(vcc));
 		pr_debug("Trying to send without setting encaps!\n");
 		kfree_skb(skb);
 		return 1;
@@ -308,9 +320,12 @@ static int pppoatm_send(struct ppp_channel *chan, struct sk_buff *skb)
 	ATM_SKB(skb)->atm_options = ATM_SKB(skb)->vcc->atm_options;
 	pr_debug("atm_skb(%p)->vcc(%p)->dev(%p)\n",
 		 skb, ATM_SKB(skb)->vcc, ATM_SKB(skb)->vcc->dev);
-	return ATM_SKB(skb)->vcc->send(ATM_SKB(skb)->vcc, skb)
+	ret = ATM_SKB(skb)->vcc->send(ATM_SKB(skb)->vcc, skb)
 	    ? DROP_PACKET : 1;
+	bh_unlock_sock(sk_atm(vcc));
+	return ret;
 nospace:
+	bh_unlock_sock(sk_atm(vcc));
 	/*
 	 * We don't have space to send this SKB now, but we might have
 	 * already applied SC_COMP_PROT compression, so may need to undo
-- 
1.8.0

[PATCH 05/17] pppoatm: drop frames to not-ready vcc

[David Woodhouse]

From: Krzysztof Mazur <krzysiek@podlesie.net>

Patches "atm: detach protocol before closing vcc"
and "pppoatm: allow assign only on a connected socket" fixed
common cases where the pppoatm_send() crashes while sending
frame to not-ready vcc. However there are still some other cases
where we can send frames to vcc, which is flagged as ATM_VF_CLOSE
(for instance after vcc_release_async()) or it's opened but not
ready yet.

Now pppoatm_send(), like vcc_sendmsg(), checks for vcc flags that
indicate that vcc is not ready. If the vcc is not ready we just
drop frame. Queueing frames is much more complicated because we
don't have callbacks that inform us about vcc flags changes.

Signed-off-by: Krzysztof Mazur <krzysiek@podlesie.net>
Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
---
 net/atm/pppoatm.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/net/atm/pppoatm.c b/net/atm/pppoatm.c
index c4a57bc..aeb726c 100644
--- a/net/atm/pppoatm.c
+++ b/net/atm/pppoatm.c
@@ -284,6 +284,13 @@ static int pppoatm_send(struct ppp_channel *chan, struct sk_buff *skb)
 	bh_lock_sock(sk_atm(vcc));
 	if (sock_owned_by_user(sk_atm(vcc)))
 		goto nospace;
+	if (test_bit(ATM_VF_RELEASED, &vcc->flags) ||
+	    test_bit(ATM_VF_CLOSE, &vcc->flags) ||
+	    !test_bit(ATM_VF_READY, &vcc->flags)) {
+		bh_unlock_sock(sk_atm(vcc));
+		kfree_skb(skb);
+		return DROP_PACKET;
+	}
 
 	switch (pvcc->encaps) {		/* LLC encapsulation needed */
 	case e_llc:
-- 
1.8.0

[PATCH 06/17] pppoatm: do not inline pppoatm_may_send()

[David Woodhouse]

From: Krzysztof Mazur <krzysiek@podlesie.net>

The pppoatm_may_send() is quite heavy and it's called three times
in pppoatm_send() and inlining costs more than 200 bytes of code
(more than 10% of total pppoatm driver code size).

add/remove: 1/0 grow/shrink: 0/1 up/down: 132/-367 (-235)
function                                     old     new   delta
pppoatm_may_send                               -     132    +132
pppoatm_send                                 900     533    -367

Signed-off-by: Krzysztof Mazur <krzysiek@podlesie.net>
Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
---
 net/atm/pppoatm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/atm/pppoatm.c b/net/atm/pppoatm.c
index aeb726c..3dce84a 100644
--- a/net/atm/pppoatm.c
+++ b/net/atm/pppoatm.c
@@ -214,7 +214,7 @@ error:
 	ppp_input_error(&pvcc->chan, 0);
 }
 
-static inline int pppoatm_may_send(struct pppoatm_vcc *pvcc, int size)
+static int pppoatm_may_send(struct pppoatm_vcc *pvcc, int size)
 {
 	/*
 	 * It's not clear that we need to bother with using atm_may_send()
-- 
1.8.0

[PATCH 07/17] solos-pci: Wait for pending TX to complete when releasing vcc

[David Woodhouse]

From: David Woodhouse <David.Woodhouse@intel.com>

We should no longer be calling the old pop routine for the vcc, after
vcc_release() has completed. Make sure we wait for any pending TX skbs
to complete, by waiting for our own PKT_PCLOSE control skb to be sent.

Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
---
 drivers/atm/solos-pci.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/atm/solos-pci.c b/drivers/atm/solos-pci.c
index 9851093..387052d 100644
--- a/drivers/atm/solos-pci.c
+++ b/drivers/atm/solos-pci.c
@@ -92,6 +92,7 @@ struct pkt_hdr {
 };
 
 struct solos_skb_cb {
+	struct completion c;
 	struct atm_vcc *vcc;
 	uint32_t dma_addr;
 };
@@ -881,11 +882,18 @@ static void pclose(struct atm_vcc *vcc)
 	header->vci = cpu_to_le16(vcc->vci);
 	header->type = cpu_to_le16(PKT_PCLOSE);
 
+	init_completion(&SKB_CB(skb)->c);
+
 	fpga_queue(card, SOLOS_CHAN(vcc->dev), skb, NULL);
 
 	clear_bit(ATM_VF_ADDR, &vcc->flags);
 	clear_bit(ATM_VF_READY, &vcc->flags);
 
+	if (!wait_for_completion_timeout(&SKB_CB(skb)->c,
+					 msecs_to_jiffies(5000)))
+		dev_warn(&card->dev->dev, "Timeout waiting for VCC close on port %d\n",
+			 SOLOS_CHAN(vcc->dev));
+
 	/* Hold up vcc_destroy_socket() (our caller) until solos_bh() in the
 	   tasklet has finished processing any incoming packets (and, more to
 	   the point, using the vcc pointer). */
@@ -1011,9 +1019,12 @@ static uint32_t fpga_tx(struct solos_card *card)
 			if (vcc) {
 				atomic_inc(&vcc->stats->tx);
 				solos_pop(vcc, oldskb);
-			} else
+			} else {
+				struct pkt_hdr *header = (void *)oldskb->data;
+				if (le16_to_cpu(header->type) == PKT_PCLOSE)
+					complete(&SKB_CB(oldskb)->c);
 				dev_kfree_skb_irq(oldskb);
-
+			}
 		}
 	}
 	/* For non-DMA TX, write the 'TX start' bit for all four ports simultaneously */
@@ -1345,6 +1356,8 @@ static struct pci_driver fpga_driver = {
 
 static int __init solos_pci_init(void)
 {
+	BUILD_BUG_ON(sizeof(struct solos_skb_cb) > sizeof(((struct sk_buff *)0)->cb));
+
 	printk(KERN_INFO "Solos PCI Driver Version %s\n", VERSION);
 	return pci_register_driver(&fpga_driver);
 }
-- 
1.8.0

[PATCH 08/17] br2684: don't send frames on not-ready vcc

[David Woodhouse]

Avoid submitting packets to a vcc which is being closed. Things go badly
wrong when the ->pop method gets later called after everything's been
torn down.

Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
Acked-by: Krzysztof Mazur <krzysiek@podlesie.net>
---
 net/atm/br2684.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/atm/br2684.c b/net/atm/br2684.c
index 8eb6fbe..c483021 100644
--- a/net/atm/br2684.c
+++ b/net/atm/br2684.c
@@ -249,6 +249,12 @@ static int br2684_xmit_vcc(struct sk_buff *skb, struct net_device *dev,
 	skb_debug(skb);
 
 	ATM_SKB(skb)->vcc = atmvcc = brvcc->atmvcc;
+	if (test_bit(ATM_VF_RELEASED, &atmvcc->flags) ||
+	    test_bit(ATM_VF_CLOSE, &atmvcc->flags) ||
+	    !test_bit(ATM_VF_READY, &atmvcc->flags)) {
+		dev_kfree_skb(skb);
+		return 0;
+	}
 	pr_debug("atm_skb(%p)->vcc(%p)->dev(%p)\n", skb, atmvcc, atmvcc->dev);
 	atomic_add(skb->truesize, &sk_atm(atmvcc)->sk_wmem_alloc);
 	ATM_SKB(skb)->atm_options = atmvcc->atm_options;
-- 
1.8.0

[PATCH 09/17] atm: Add release_cb() callback to vcc

[David Woodhouse]

From: David Woodhouse <David.Woodhouse@intel.com>

The immediate use case for this is that it will allow us to ensure that a
pppoatm queue is woken after it has to drop a packet due to the sock being
locked.

Note that 'release_cb' is called when the socket is *unlocked*. This is
not to be confused with vcc_release() — which probably ought to be called
vcc_close().

Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
Acked-by: Krzysztof Mazur <krzysiek@podlesie.net>
---
 include/linux/atmdev.h |  1 +
 net/atm/common.c       | 10 ++++++++++
 2 files changed, 11 insertions(+)

diff --git a/include/linux/atmdev.h b/include/linux/atmdev.h
index 72db2af..c1da539 100644
--- a/include/linux/atmdev.h
+++ b/include/linux/atmdev.h
@@ -99,6 +99,7 @@ struct atm_vcc {
 	struct atm_dev	*dev;		/* device back pointer */
 	struct atm_qos	qos;		/* QOS */
 	struct atm_sap	sap;		/* SAP */
+	void (*release_cb)(struct atm_vcc *vcc); /* release_sock callback */
 	void (*push)(struct atm_vcc *vcc,struct sk_buff *skb);
 	void (*pop)(struct atm_vcc *vcc,struct sk_buff *skb); /* optional */
 	int (*push_oam)(struct atm_vcc *vcc,void *cell);
diff --git a/net/atm/common.c b/net/atm/common.c
index 2421664..806fc0a 100644
--- a/net/atm/common.c
+++ b/net/atm/common.c
@@ -126,10 +126,19 @@ static void vcc_write_space(struct sock *sk)
 	rcu_read_unlock();
 }
 
+static void vcc_release_cb(struct sock *sk)
+{
+	struct atm_vcc *vcc = atm_sk(sk);
+
+	if (vcc->release_cb)
+		vcc->release_cb(vcc);
+}
+
 static struct proto vcc_proto = {
 	.name	  = "VCC",
 	.owner	  = THIS_MODULE,
 	.obj_size = sizeof(struct atm_vcc),
+	.release_cb = vcc_release_cb,
 };
 
 int vcc_create(struct net *net, struct socket *sock, int protocol, int family)
@@ -158,6 +167,7 @@ int vcc_create(struct net *net, struct socket *sock, int protocol, int family)
 	vcc->pop = NULL;
 	vcc->owner = NULL;
 	vcc->push_oam = NULL;
+	vcc->release_cb = NULL;
 	vcc->vpi = vcc->vci = 0; /* no VCI/VPI yet */
 	vcc->atm_options = vcc->aal_options = 0;
 	sk->sk_destruct = vcc_sock_destruct;
-- 
1.8.0

[PATCH 10/17] pppoatm: fix missing wakeup in pppoatm_send()

[David Woodhouse]

From: David Woodhouse <David.Woodhouse@intel.com>

Now that we can return zero from pppoatm_send() for reasons *other* than
the queue being full, that means we can't depend on a subsequent call to
pppoatm_pop() waking the queue, and we might leave it stalled
indefinitely.

Use the ->release_cb() callback to wake the queue after the sock is
unlocked.

Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
Acked-by: Krzysztof Mazur <krzysiek@podlesie.net>
---
 net/atm/pppoatm.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/net/atm/pppoatm.c b/net/atm/pppoatm.c
index 3dce84a..9fcda8c 100644
--- a/net/atm/pppoatm.c
+++ b/net/atm/pppoatm.c
@@ -60,6 +60,7 @@ struct pppoatm_vcc {
 	struct atm_vcc	*atmvcc;	/* VCC descriptor */
 	void (*old_push)(struct atm_vcc *, struct sk_buff *);
 	void (*old_pop)(struct atm_vcc *, struct sk_buff *);
+	void (*old_release_cb)(struct atm_vcc *);
 	struct module *old_owner;
 					/* keep old push/pop for detaching */
 	enum pppoatm_encaps encaps;
@@ -108,6 +109,14 @@ static void pppoatm_wakeup_sender(unsigned long arg)
 	ppp_output_wakeup((struct ppp_channel *) arg);
 }
 
+static void pppoatm_release_cb(struct atm_vcc *atmvcc)
+{
+	struct pppoatm_vcc *pvcc = atmvcc_to_pvcc(atmvcc);
+
+	tasklet_schedule(&pvcc->wakeup_tasklet);
+	if (pvcc->old_release_cb)
+		pvcc->old_release_cb(atmvcc);
+}
 /*
  * This gets called every time the ATM card has finished sending our
  * skb.  The ->old_pop will take care up normal atm flow control,
@@ -152,6 +161,7 @@ static void pppoatm_unassign_vcc(struct atm_vcc *atmvcc)
 	pvcc = atmvcc_to_pvcc(atmvcc);
 	atmvcc->push = pvcc->old_push;
 	atmvcc->pop = pvcc->old_pop;
+	atmvcc->release_cb = pvcc->old_release_cb;
 	tasklet_kill(&pvcc->wakeup_tasklet);
 	ppp_unregister_channel(&pvcc->chan);
 	atmvcc->user_back = NULL;
@@ -388,6 +398,7 @@ static int pppoatm_assign_vcc(struct atm_vcc *atmvcc, void __user *arg)
 	pvcc->old_push = atmvcc->push;
 	pvcc->old_pop = atmvcc->pop;
 	pvcc->old_owner = atmvcc->owner;
+	pvcc->old_release_cb = atmvcc->release_cb;
 	pvcc->encaps = (enum pppoatm_encaps) be.encaps;
 	pvcc->chan.private = pvcc;
 	pvcc->chan.ops = &pppoatm_ops;
@@ -403,6 +414,7 @@ static int pppoatm_assign_vcc(struct atm_vcc *atmvcc, void __user *arg)
 	atmvcc->user_back = pvcc;
 	atmvcc->push = pppoatm_push;
 	atmvcc->pop = pppoatm_pop;
+	atmvcc->release_cb = pppoatm_release_cb;
 	__module_get(THIS_MODULE);
 	atmvcc->owner = THIS_MODULE;
 
-- 
1.8.0

[PATCH 11/17] br2684: fix module_put() race

[David Woodhouse]

From: David Woodhouse <David.Woodhouse@intel.com>

The br2684 code used module_put() during unassignment from vcc with
hope that we have BKL. This assumption is no longer true.

Now owner field in atmvcc is used to move this module_put()
to vcc_destroy_socket().

Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
Acked-by: Krzysztof Mazur <krzysiek@podlesie.net>
---
 net/atm/br2684.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/atm/br2684.c b/net/atm/br2684.c
index c483021..373d784 100644
--- a/net/atm/br2684.c
+++ b/net/atm/br2684.c
@@ -68,6 +68,7 @@ struct br2684_vcc {
 	/* keep old push, pop functions for chaining */
 	void (*old_push)(struct atm_vcc *vcc, struct sk_buff *skb);
 	void (*old_pop)(struct atm_vcc *vcc, struct sk_buff *skb);
+	struct module *old_owner;
 	enum br2684_encaps encaps;
 	struct list_head brvccs;
 #ifdef CONFIG_ATM_BR2684_IPFILTER
@@ -385,8 +386,8 @@ static void br2684_close_vcc(struct br2684_vcc *brvcc)
 	write_unlock_irq(&devs_lock);
 	brvcc->atmvcc->user_back = NULL;	/* what about vcc->recvq ??? */
 	brvcc->old_push(brvcc->atmvcc, NULL);	/* pass on the bad news */
+	module_put(brvcc->old_owner);
 	kfree(brvcc);
-	module_put(THIS_MODULE);
 }
 
 /* when AAL5 PDU comes in: */
@@ -560,9 +561,11 @@ static int br2684_regvcc(struct atm_vcc *atmvcc, void __user * arg)
 	brvcc->encaps = (enum br2684_encaps)be.encaps;
 	brvcc->old_push = atmvcc->push;
 	brvcc->old_pop = atmvcc->pop;
+	brvcc->old_owner = atmvcc->owner;
 	barrier();
 	atmvcc->push = br2684_push;
 	atmvcc->pop = br2684_pop;
+	atmvcc->owner = THIS_MODULE;
 
 	/* initialize netdev carrier state */
 	if (atmvcc->dev->signal == ATM_PHY_SIG_LOST)
-- 
1.8.0

[PATCH 12/17] solos-pci: Fix leak of skb received for unknown vcc

[David Woodhouse]

From: Nathan Williams <nathan@traverse.com.au>

... and ensure that the next skb is set up for RX in the DMA case.

Signed-off-by: Nathan Williams <nathan@traverse.com.au>
Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
---
 drivers/atm/solos-pci.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/atm/solos-pci.c b/drivers/atm/solos-pci.c
index 387052d..6258961 100644
--- a/drivers/atm/solos-pci.c
+++ b/drivers/atm/solos-pci.c
@@ -711,7 +711,8 @@ void solos_bh(unsigned long card_arg)
 						dev_warn(&card->dev->dev, "Received packet for unknown VPI.VCI %d.%d on port %d\n",
 							 le16_to_cpu(header->vpi), le16_to_cpu(header->vci),
 							 port);
-					continue;
+					dev_kfree_skb_any(skb);
+					break;
 				}
 				atm_charge(vcc, skb->truesize);
 				vcc->push(vcc, skb);
-- 
1.8.0

[PATCH 13/17] br2684: allow assign only on a connected socket

[David Woodhouse]

From: Krzysztof Mazur <krzysiek@podlesie.net>

The br2684 does not check if used vcc is in connected state,
causing potential Oops in pppoatm_send() when vcc->send() is called
on not fully connected socket.

Now br2684 can be assigned only on connected sockets; otherwise
-EINVAL error is returned.

Signed-off-by: Krzysztof Mazur <krzysiek@podlesie.net>
Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
---
 net/atm/br2684.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/net/atm/br2684.c b/net/atm/br2684.c
index 373d784..a4ee4ad 100644
--- a/net/atm/br2684.c
+++ b/net/atm/br2684.c
@@ -704,10 +704,13 @@ static int br2684_ioctl(struct socket *sock, unsigned int cmd,
 			return -ENOIOCTLCMD;
 		if (!capable(CAP_NET_ADMIN))
 			return -EPERM;
-		if (cmd == ATM_SETBACKEND)
+		if (cmd == ATM_SETBACKEND) {
+			if (sock->state != SS_CONNECTED)
+				return -EINVAL;
 			return br2684_regvcc(atmvcc, argp);
-		else
+		} else {
 			return br2684_create(argp);
+		}
 #ifdef CONFIG_ATM_BR2684_IPFILTER
 	case BR2684_SETFILT:
 		if (atmvcc->push != br2684_push)
-- 
1.8.0

[PATCH 14/17] pppoatm: optimise PPP channel wakeups after sock_owned_by_user()

[David Woodhouse]

From: David Woodhouse <David.Woodhouse@intel.com>

We don't need to schedule the wakeup tasklet on *every* unlock; only if we
actually blocked the channel in the first place.

Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
Acked-by: Krzysztof Mazur <krzysiek@podlesie.net>
---
 net/atm/pppoatm.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/net/atm/pppoatm.c b/net/atm/pppoatm.c
index 9fcda8c..8c93267 100644
--- a/net/atm/pppoatm.c
+++ b/net/atm/pppoatm.c
@@ -113,7 +113,17 @@ static void pppoatm_release_cb(struct atm_vcc *atmvcc)
 {
 	struct pppoatm_vcc *pvcc = atmvcc_to_pvcc(atmvcc);
 
-	tasklet_schedule(&pvcc->wakeup_tasklet);
+	/*
+	 * As in pppoatm_pop(), it's safe to clear the BLOCKED bit here because
+	 * the wakeup *can't* race with pppoatm_send(). They both hold the PPP
+	 * channel's ->downl lock. And the potential race with *setting* it,
+	 * which leads to the double-check dance in pppoatm_may_send(), doesn't
+	 * exist here. In the sock_owned_by_user() case in pppoatm_send(), we
+	 * set the BLOCKED bit while the socket is still locked. We know that
+	 * ->release_cb() can't be called until that's done.
+	 */
+	if (test_and_clear_bit(BLOCKED, &pvcc->blocked))
+		tasklet_schedule(&pvcc->wakeup_tasklet);
 	if (pvcc->old_release_cb)
 		pvcc->old_release_cb(atmvcc);
 }
@@ -292,8 +302,15 @@ static int pppoatm_send(struct ppp_channel *chan, struct sk_buff *skb)
 
 	vcc = ATM_SKB(skb)->vcc;
 	bh_lock_sock(sk_atm(vcc));
-	if (sock_owned_by_user(sk_atm(vcc)))
+	if (sock_owned_by_user(sk_atm(vcc))) {
+		/*
+		 * Needs to happen (and be flushed, hence test_and_) before we unlock
+		 * the socket. It needs to be seen by the time our ->release_cb gets
+		 * called.
+		 */
+		test_and_set_bit(BLOCKED, &pvcc->blocked);
 		goto nospace;
+	}
 	if (test_bit(ATM_VF_RELEASED, &vcc->flags) ||
 	    test_bit(ATM_VF_CLOSE, &vcc->flags) ||
 	    !test_bit(ATM_VF_READY, &vcc->flags)) {
-- 
1.8.0

[PATCH 15/17] solos-pci: clean up pclose() function

[David Woodhouse]

From: David Woodhouse <David.Woodhouse@intel.com>

 - Flush pending TX skbs from the queue rather than waiting for them all to
   complete (suggested by Krzysztof Mazur <krzysiek@podlesie.net>).
 - Clear ATM_VF_ADDR only when the PKT_PCLOSE packet has been submitted.
 - Don't clear ATM_VF_READY at all — vcc_destroy_socket() does that for us.

Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
---
 drivers/atm/solos-pci.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/drivers/atm/solos-pci.c b/drivers/atm/solos-pci.c
index 6258961..7c56286 100644
--- a/drivers/atm/solos-pci.c
+++ b/drivers/atm/solos-pci.c
@@ -868,9 +868,20 @@ static int popen(struct atm_vcc *vcc)
 static void pclose(struct atm_vcc *vcc)
 {
 	struct solos_card *card = vcc->dev->dev_data;
-	struct sk_buff *skb;
+	unsigned char port = SOLOS_CHAN(vcc->dev);
+	struct sk_buff *skb, *tmpskb;
 	struct pkt_hdr *header;
 
+	/* Remove any yet-to-be-transmitted packets from the pending queue */
+	spin_lock(&card->tx_queue_lock);
+	skb_queue_walk_safe(&card->tx_queue[port], skb, tmpskb) {
+		if (SKB_CB(skb)->vcc == vcc) {
+			skb_unlink(skb, &card->tx_queue[port]);
+			solos_pop(vcc, skb);
+		}
+	}
+	spin_unlock(&card->tx_queue_lock);
+
 	skb = alloc_skb(sizeof(*header), GFP_ATOMIC);
 	if (!skb) {
 		dev_warn(&card->dev->dev, "Failed to allocate sk_buff in pclose()\n");
@@ -885,20 +896,19 @@ static void pclose(struct atm_vcc *vcc)
 
 	init_completion(&SKB_CB(skb)->c);
 
-	fpga_queue(card, SOLOS_CHAN(vcc->dev), skb, NULL);
-
-	clear_bit(ATM_VF_ADDR, &vcc->flags);
-	clear_bit(ATM_VF_READY, &vcc->flags);
+	fpga_queue(card, port, skb, NULL);
 
-	if (!wait_for_completion_timeout(&SKB_CB(skb)->c,
-					 msecs_to_jiffies(5000)))
+	if (!wait_for_completion_timeout(&SKB_CB(skb)->c, 5 * HZ))
 		dev_warn(&card->dev->dev, "Timeout waiting for VCC close on port %d\n",
-			 SOLOS_CHAN(vcc->dev));
+			 port);
 
 	/* Hold up vcc_destroy_socket() (our caller) until solos_bh() in the
 	   tasklet has finished processing any incoming packets (and, more to
 	   the point, using the vcc pointer). */
 	tasklet_unlock_wait(&card->tlet);
+
+	clear_bit(ATM_VF_ADDR, &vcc->flags);
+
 	return;
 }
 
-- 
1.8.0

[PATCH 16/17] solos-pci: use GFP_KERNEL where possible, not GFP_ATOMIC

[David Woodhouse]

From: David Woodhouse <David.Woodhouse@intel.com>

Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
---
 drivers/atm/solos-pci.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/atm/solos-pci.c b/drivers/atm/solos-pci.c
index 7c56286..af19202 100644
--- a/drivers/atm/solos-pci.c
+++ b/drivers/atm/solos-pci.c
@@ -842,7 +842,7 @@ static int popen(struct atm_vcc *vcc)
 		return -EINVAL;
 	}
 
-	skb = alloc_skb(sizeof(*header), GFP_ATOMIC);
+	skb = alloc_skb(sizeof(*header), GFP_KERNEL);
 	if (!skb) {
 		if (net_ratelimit())
 			dev_warn(&card->dev->dev, "Failed to allocate sk_buff in popen()\n");
@@ -882,7 +882,7 @@ static void pclose(struct atm_vcc *vcc)
 	}
 	spin_unlock(&card->tx_queue_lock);
 
-	skb = alloc_skb(sizeof(*header), GFP_ATOMIC);
+	skb = alloc_skb(sizeof(*header), GFP_KERNEL);
 	if (!skb) {
 		dev_warn(&card->dev->dev, "Failed to allocate sk_buff in pclose()\n");
 		return;
@@ -1270,7 +1270,7 @@ static int atm_init(struct solos_card *card, struct device *parent)
 		card->atmdev[i]->phy_data = (void *)(unsigned long)i;
 		atm_dev_signal_change(card->atmdev[i], ATM_PHY_SIG_FOUND);
 
-		skb = alloc_skb(sizeof(*header), GFP_ATOMIC);
+		skb = alloc_skb(sizeof(*header), GFP_KERNEL);
 		if (!skb) {
 			dev_warn(&card->dev->dev, "Failed to allocate sk_buff in atm_init()\n");
 			continue;
-- 
1.8.0

[PATCH 17/17] solos-pci: remove list_vccs() debugging function

[David Woodhouse]

From: David Woodhouse <David.Woodhouse@intel.com>

No idea why we've gone so long dumping a list of VCCs with vci==0 on
every ->open() call...

Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
---
 drivers/atm/solos-pci.c | 41 -----------------------------------------
 1 file changed, 41 deletions(-)

diff --git a/drivers/atm/solos-pci.c b/drivers/atm/solos-pci.c
index af19202..e59bcfd 100644
--- a/drivers/atm/solos-pci.c
+++ b/drivers/atm/solos-pci.c
@@ -165,7 +165,6 @@ static void fpga_queue(struct solos_card *card, int port, struct sk_buff *skb,
 static uint32_t fpga_tx(struct solos_card *);
 static irqreturn_t solos_irq(int irq, void *dev_id);
 static struct atm_vcc* find_vcc(struct atm_dev *dev, short vpi, int vci);
-static int list_vccs(int vci);
 static int atm_init(struct solos_card *, struct device *);
 static void atm_remove(struct solos_card *);
 static int send_command(struct solos_card *card, int dev, const char *buf, size_t size);
@@ -792,44 +791,6 @@ static struct atm_vcc *find_vcc(struct atm_dev *dev, short vpi, int vci)
 	return vcc;
 }
 
-static int list_vccs(int vci)
-{
-	struct hlist_head *head;
-	struct atm_vcc *vcc;
-	struct hlist_node *node;
-	struct sock *s;
-	int num_found = 0;
-	int i;
-
-	read_lock(&vcc_sklist_lock);
-	if (vci != 0){
-		head = &vcc_hash[vci & (VCC_HTABLE_SIZE -1)];
-		sk_for_each(s, node, head) {
-			num_found ++;
-			vcc = atm_sk(s);
-			printk(KERN_DEBUG "Device: %d Vpi: %d Vci: %d\n",
-			       vcc->dev->number,
-			       vcc->vpi,
-			       vcc->vci);
-		}
-	} else {
-		for(i = 0; i < VCC_HTABLE_SIZE; i++){
-			head = &vcc_hash[i];
-			sk_for_each(s, node, head) {
-				num_found ++;
-				vcc = atm_sk(s);
-				printk(KERN_DEBUG "Device: %d Vpi: %d Vci: %d\n",
-				       vcc->dev->number,
-				       vcc->vpi,
-				       vcc->vci);
-			}
-		}
-	}
-	read_unlock(&vcc_sklist_lock);
-	return num_found;
-}
-
-
 static int popen(struct atm_vcc *vcc)
 {
 	struct solos_card *card = vcc->dev->dev_data;
@@ -859,8 +820,6 @@ static int popen(struct atm_vcc *vcc)
 
 	set_bit(ATM_VF_ADDR, &vcc->flags);
 	set_bit(ATM_VF_READY, &vcc->flags);
-	list_vccs(0);
-
 
 	return 0;
 }
-- 
1.8.0

Re: [PATCH 05/17] pppoatm: drop frames to not-ready vcc

[Krzysztof Mazur]

On Fri, Nov 30, 2012 at 12:35:24AM +0000, David Woodhouse wrote:
> From: Krzysztof Mazur <krzysiek@podlesie.net>
> 
> Patches "atm: detach protocol before closing vcc"
> and "pppoatm: allow assign only on a connected socket" fixed
> common cases where the pppoatm_send() crashes while sending
> frame to not-ready vcc. However there are still some other cases
> where we can send frames to vcc, which is flagged as ATM_VF_CLOSE
> (for instance after vcc_release_async()) or it's opened but not
> ready yet.

We should update the paragraph above, with something like:

The vcc_destroy_socket() closes vcc before the protocol is detached
from vcc by calling vcc->push() with NULL skb. This leaves some time
window, where the protocol may call vcc->send() on closed vcc
and crash.

> 
> Now pppoatm_send(), like vcc_sendmsg(), checks for vcc flags that
> indicate that vcc is not ready. If the vcc is not ready we just
> drop frame. Queueing frames is much more complicated because we
> don't have callbacks that inform us about vcc flags changes.
> 
> Signed-off-by: Krzysztof Mazur <krzysiek@podlesie.net>
> Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>

I'm sending the updated version. The only difference is the commit
message.

Krzysiek
-- >8 --
Subject: [PATCH 05/17] pppoatm: drop frames to not-ready vcc

The vcc_destroy_socket() closes vcc before the protocol is detached
from vcc by calling vcc->push() with NULL skb. This leaves some time
window, where the protocol may call vcc->send() on closed vcc
and crash.

Now pppoatm_send(), like vcc_sendmsg(), checks for vcc flags that
indicate that vcc is not ready. If the vcc is not ready we just
drop frame. Queueing frames is much more complicated because we
don't have callbacks that inform us about vcc flags changes.

Signed-off-by: Krzysztof Mazur <krzysiek@podlesie.net>
---
 net/atm/pppoatm.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/net/atm/pppoatm.c b/net/atm/pppoatm.c
index c4a57bc..aeb726c 100644
--- a/net/atm/pppoatm.c
+++ b/net/atm/pppoatm.c
@@ -284,6 +284,13 @@ static int pppoatm_send(struct ppp_channel *chan, struct sk_buff *skb)
 	bh_lock_sock(sk_atm(vcc));
 	if (sock_owned_by_user(sk_atm(vcc)))
 		goto nospace;
+	if (test_bit(ATM_VF_RELEASED, &vcc->flags) ||
+	    test_bit(ATM_VF_CLOSE, &vcc->flags) ||
+	    !test_bit(ATM_VF_READY, &vcc->flags)) {
+		bh_unlock_sock(sk_atm(vcc));
+		kfree_skb(skb);
+		return DROP_PACKET;
+	}
 
 	switch (pvcc->encaps) {		/* LLC encapsulation needed */
 	case e_llc:
-- 
1.8.0.411.g71a7da8

Re: [PATCH 00/17] ATM fixes for pppoatm/br2684

[Krzysztof Mazur]

On Fri, Nov 30, 2012 at 12:35:19AM +0000, David Woodhouse wrote:
> This is the result of pulling on the thread started by Krzysztof Mazur's
> original patch 'pppoatm: don't send frames to destroyed vcc'.
> 
> Various problems in the pppoatm and br2684 code are solved, some of which
> were easily triggered and would panic the kernel.
> 
> The patch series can be pulled from
> 	git://git.infradead.org/users/dwmw2/atm.git
> or viewed at 
> 	http://git.infradead.org/users/dwmw2/atm.git
> 
> DaveM, please wait for an ack from Krzysztof and Chas before pulling this.

looks good to me, except "pppoatm: drop frames to not-ready vcc"
that needs updated commit message update after we drop "1/7".

Thanks,

Krzysiek

Re: [PATCH 00/17] ATM fixes for pppoatm/br2684

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1955 bytes --]

On Fri, 2012-11-30 at 11:44 +0100, Krzysztof Mazur wrote:
> > The patch series can be pulled from
> >       git://git.infradead.org/users/dwmw2/atm.git
> > or viewed at 
> >       http://git.infradead.org/users/dwmw2/atm.git
> > 
> > DaveM, please wait for an ack from Krzysztof and Chas before pulling this.
> 
> looks good to me, except [<fixed>]

On Fri, 2012-11-30 at 12:12 -0500, chas williams - CONTRACTOR wrote:
> no objections.  i think this deals with my concerns. 

Dave, if you're not now ignoring this thread entirely, please pull into
net-next from
	git://git.infradead.org/users/dwmw2/atm.git

David Woodhouse (9):
      solos-pci: wait for pending TX to complete when releasing vcc
      atm: add release_cb() callback to vcc
      br2684: don't send frames on not-ready vcc
      pppoatm: fix missing wakeup in pppoatm_send()
      br2684: fix module_put() race
      pppoatm: optimise PPP channel wakeups after sock_owned_by_user()
      solos-pci: clean up pclose() function
      solos-pci: use GFP_KERNEL where possible, not GFP_ATOMIC
      solos-pci: remove list_vccs() debugging function

Krzysztof Mazur (7):
      atm: add owner of push() callback to atmvcc
      pppoatm: allow assign only on a connected socket
      pppoatm: fix module_put() race
      pppoatm: take ATM socket lock in pppoatm_send()
      pppoatm: drop frames to not-ready vcc
      pppoatm: do not inline pppoatm_may_send()
      br2684: allow assign only on a connected socket

Nathan Williams (1):
      solos-pci: Fix leak of skb received for unknown vcc

 drivers/atm/solos-pci.c | 85 ++++++++++++++++++++-----------------------------
 include/linux/atmdev.h  |  2 ++
 net/atm/br2684.c        | 55 ++++++++++++++++++++++++++++----
 net/atm/common.c        | 12 +++++++
 net/atm/pppoatm.c       | 68 ++++++++++++++++++++++++++++++++++++---
 5 files changed, 160 insertions(+), 62 deletions(-)

-- 
dwmw2


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6171 bytes --]

Re: [PATCH 00/17] ATM fixes for pppoatm/br2684

[David Miller]

From: David Woodhouse <dwmw2@infradead.org>
Date: Fri, 30 Nov 2012 20:22:09 +0000

> Dave, if you're not now ignoring this thread entirely, please pull into
> net-next from
> 	git://git.infradead.org/users/dwmw2/atm.git

Can you make this actually build first?

drivers/atm/ambassador.c: In function ‘ucode_init’:
drivers/atm/ambassador.c:1972:19: warning: ‘fw’ may be used uninitialized in this function [-Wmaybe-uninitialized]
drivers/atm/solos-pci.c: In function ‘solos_pci_init’:
drivers/atm/solos-pci.c:1329:2: error: size of unnamed array is negative

Re: [PATCH 00/17] ATM fixes for pppoatm/br2684

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Sat, 01 Dec 2012 11:43:20 -0500 (EST)

> From: David Woodhouse <dwmw2@infradead.org>
> Date: Fri, 30 Nov 2012 20:22:09 +0000
> 
>> Dave, if you're not now ignoring this thread entirely, please pull into
>> net-next from
>> 	git://git.infradead.org/users/dwmw2/atm.git
> 
> Can you make this actually build first?
> 
> drivers/atm/ambassador.c: In function ‘ucode_init’:
> drivers/atm/ambassador.c:1972:19: warning: ‘fw’ may be used uninitialized in this function [-Wmaybe-uninitialized]
> drivers/atm/solos-pci.c: In function ‘solos_pci_init’:
> drivers/atm/solos-pci.c:1329:2: error: size of unnamed array is negative

It's from adding the completion to the solos skb cb, you can't do
that.  It won't fit on 64-bit when all debugging kconfig options are
enabled.

 

Re: [PATCH 00/17] ATM fixes for pppoatm/br2684

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 404 bytes --]

On Sat, 2012-12-01 at 11:44 -0500, David Miller wrote:
> It's from adding the completion to the solos skb cb, you can't do
> that.  It won't fit on 64-bit when all debugging kconfig options are
> enabled.

Aha, thanks. Will sort that out. Apologies.

The ambassador one may be related to a separate patch which fixes the
endless loop in firmware loading? Not in my tree, though.

-- 
dwmw2


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6171 bytes --]

Re: [PATCH 00/17] ATM fixes for pppoatm/br2684

[Chas Williams (CONTRACTOR)]

In message <1354380532.21562.345.camel@shinybook.infradead.org>,David Woodhouse writes:
>On Sat, 2012-12-01 at 11:44 -0500, David Miller wrote:
>> It's from adding the completion to the solos skb cb, you can't do
>> that.  It won't fit on 64-bit when all debugging kconfig options are
>> enabled.
>
>Aha, thanks. Will sort that out. Apologies.
>
>The ambassador one may be related to a separate patch which fixes the
>endless loop in firmware loading? Not in my tree, though.

i think the warning with the ambassador dates back to before the most
recent patch.  probably to the conversion to use the firmware loader.

Re: [PATCH 00/17] ATM fixes for pppoatm/br2684

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 984 bytes --]

On Sat, 2012-12-01 at 12:02 -0500, Chas Williams (CONTRACTOR) wrote:
> In message <1354380532.21562.345.camel@shinybook.infradead.org>,David Woodhouse writes:
> >On Sat, 2012-12-01 at 11:44 -0500, David Miller wrote:
> >> It's from adding the completion to the solos skb cb, you can't do
> >> that.  It won't fit on 64-bit when all debugging kconfig options are
> >> enabled.
> >
> >Aha, thanks. Will sort that out. Apologies.
> >
> >The ambassador one may be related to a separate patch which fixes the
> >endless loop in firmware loading? Not in my tree, though.
> 
> i think the warning with the ambassador dates back to before the most
> recent patch.  probably to the conversion to use the firmware loader.

Possibly not even a bug at all, in fact — GCC is fairly loose with those
warnings. But if it is a bug it's my fault. I'll take a look at that
too. Not tonight though; I'm going out shortly and will only just manage
the solos-pci fix.

-- 
dwmw2


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6171 bytes --]

Re: [PATCH 00/17] ATM fixes for pppoatm/br2684

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 2136 bytes --]

On Sat, 2012-12-01 at 11:44 -0500, David Miller wrote:
> 
> > drivers/atm/solos-pci.c: In function ‘solos_pci_init’:
> > drivers/atm/solos-pci.c:1329:2: error: size of unnamed array is
> negative
> 
> It's from adding the completion to the solos skb cb, you can't do
> that.  It won't fit on 64-bit when all debugging kconfig options are
> enabled.

This is a sane use of skb refcounting, right? It seems to work, so if
it's OK I'll redo the offending patch with this change to avoid breaking
the bisection, and submit a new pull request.

Very glad I added the BUILD_BUG_ON on the cb struct size now. Perhaps
there should be a generic helper for that? Something like
 skb_cb_cast(struct foo_cb, skb) could do it automatically...?

diff --git a/drivers/atm/solos-pci.c b/drivers/atm/solos-pci.c
index e59bcfd..6fe62c5 100644
--- a/drivers/atm/solos-pci.c
+++ b/drivers/atm/solos-pci.c
@@ -92,7 +92,6 @@ struct pkt_hdr {
 };
 
 struct solos_skb_cb {
-	struct completion c;
 	struct atm_vcc *vcc;
 	uint32_t dma_addr;
 };
@@ -853,14 +852,15 @@ static void pclose(struct atm_vcc *vcc)
 	header->vci = cpu_to_le16(vcc->vci);
 	header->type = cpu_to_le16(PKT_PCLOSE);
 
-	init_completion(&SKB_CB(skb)->c);
-
+	skb_get(skb);
 	fpga_queue(card, port, skb, NULL);
 
-	if (!wait_for_completion_timeout(&SKB_CB(skb)->c, 5 * HZ))
+	if (!wait_event_timeout(card->param_wq, !skb_shared(skb), 5 * HZ))
 		dev_warn(&card->dev->dev, "Timeout waiting for VCC close on port %d\n",
 			 port);
 
+	dev_kfree_skb_any(skb);
+
 	/* Hold up vcc_destroy_socket() (our caller) until solos_bh() in the
 	   tasklet has finished processing any incoming packets (and, more to
 	   the point, using the vcc pointer). */
@@ -990,10 +990,8 @@ static uint32_t fpga_tx(struct solos_card *card)
 				atomic_inc(&vcc->stats->tx);
 				solos_pop(vcc, oldskb);
 			} else {
-				struct pkt_hdr *header = (void *)oldskb->data;
-				if (le16_to_cpu(header->type) == PKT_PCLOSE)
-					complete(&SKB_CB(oldskb)->c);
 				dev_kfree_skb_irq(oldskb);
+				wake_up(&card->param_wq);
 			}
 		}
 	}


-- 
dwmw2


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6171 bytes --]

[PATCH v2 07/17] solos-pci: Wait for pending TX to complete when releasing vcc

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 2566 bytes --]

We should no longer be calling the old pop routine for the vcc, after
vcc_release() has completed. Make sure we wait for any pending TX skbs
to complete, by waiting for our own PKT_PCLOSE control skb to be sent.

Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
---
v2: Don't put a struct completion into skb->cb[]. It doesn't fit, so
instead use the existing card->param_wq, hold a refcount on the skb, and
wait for the fpga_tx code to free it.

I won't repost the rest of the series; they're materially unchanged
apart from a tiny amount of massaging to make them apply with this
change, so it would just be noise.

 drivers/atm/solos-pci.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/atm/solos-pci.c b/drivers/atm/solos-pci.c
index 9851093..026bdc1 100644
--- a/drivers/atm/solos-pci.c
+++ b/drivers/atm/solos-pci.c
@@ -866,6 +866,7 @@ static int popen(struct atm_vcc *vcc)
 static void pclose(struct atm_vcc *vcc)
 {
 	struct solos_card *card = vcc->dev->dev_data;
+	unsigned char port = SOLOS_CHAN(vcc->dev);
 	struct sk_buff *skb;
 	struct pkt_hdr *header;
 
@@ -881,11 +882,18 @@ static void pclose(struct atm_vcc *vcc)
 	header->vci = cpu_to_le16(vcc->vci);
 	header->type = cpu_to_le16(PKT_PCLOSE);
 
-	fpga_queue(card, SOLOS_CHAN(vcc->dev), skb, NULL);
+	skb_get(skb);
+	fpga_queue(card, port, skb, NULL);
 
 	clear_bit(ATM_VF_ADDR, &vcc->flags);
 	clear_bit(ATM_VF_READY, &vcc->flags);
 
+	if (!wait_event_timeout(card->param_wq, !skb_shared(skb), 5 * HZ))
+		dev_warn(&card->dev->dev,
+			 "Timeout waiting for VCC close on port %d\n", port);
+
+	dev_kfree_skb(skb);
+
 	/* Hold up vcc_destroy_socket() (our caller) until solos_bh() in the
 	   tasklet has finished processing any incoming packets (and, more to
 	   the point, using the vcc pointer). */
@@ -1011,9 +1019,10 @@ static uint32_t fpga_tx(struct solos_card *card)
 			if (vcc) {
 				atomic_inc(&vcc->stats->tx);
 				solos_pop(vcc, oldskb);
-			} else
+			} else {
 				dev_kfree_skb_irq(oldskb);
-
+				wake_up(&card->param_wq);
+			}
 		}
 	}
 	/* For non-DMA TX, write the 'TX start' bit for all four ports simultaneously */
@@ -1345,6 +1354,8 @@ static struct pci_driver fpga_driver = {
 
 static int __init solos_pci_init(void)
 {
+	BUILD_BUG_ON(sizeof(struct solos_skb_cb) > sizeof(((struct sk_buff *)0)->cb));
+
 	printk(KERN_INFO "Solos PCI Driver Version %s\n", VERSION);
 	return pci_register_driver(&fpga_driver);
 }
-- 
1.8.0


-- 
dwmw2


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6171 bytes --]

Re: [PATCH 00/17] ATM fixes for pppoatm/br2684

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 2447 bytes --]

On Sat, 2012-12-01 at 11:44 -0500, David Miller wrote:
> 
> > drivers/atm/solos-pci.c: In function ‘solos_pci_init’:
> > drivers/atm/solos-pci.c:1329:2: error: size of unnamed array is
> negative
> 
> It's from adding the completion to the solos skb cb, you can't do
> that.  It won't fit on 64-bit when all debugging kconfig options are
> enabled.

Thanks for catching that. I've just posted a [v2] version of the
offending patch, which no longer puts a completion into the skb cb.

I'd appreciate a slightly more clueful eye looking over the incremental
patch (below) just to confirm that the new method is correct, but it
certainly seems to work. This version is identical to the one I posted
earlier, except that I use dev_kfree_skb() in the pclose() function
instead of dev_kfree_skb_any(). We know this will be called from a
suitable context, and it even uses GFP_KERNEL a few lines higher up.

If that's OK, please pull the resulting tree from
	git://git.infradead.org/users/dwmw2/atm.git


diff --git a/drivers/atm/solos-pci.c b/drivers/atm/solos-pci.c
index e59bcfd..6619a8a 100644
--- a/drivers/atm/solos-pci.c
+++ b/drivers/atm/solos-pci.c
@@ -92,7 +92,6 @@ struct pkt_hdr {
 };
 
 struct solos_skb_cb {
-	struct completion c;
 	struct atm_vcc *vcc;
 	uint32_t dma_addr;
 };
@@ -853,13 +852,14 @@ static void pclose(struct atm_vcc *vcc)
 	header->vci = cpu_to_le16(vcc->vci);
 	header->type = cpu_to_le16(PKT_PCLOSE);
 
-	init_completion(&SKB_CB(skb)->c);
-
+	skb_get(skb);
 	fpga_queue(card, port, skb, NULL);
 
-	if (!wait_for_completion_timeout(&SKB_CB(skb)->c, 5 * HZ))
-		dev_warn(&card->dev->dev, "Timeout waiting for VCC close on port %d\n",
-			 port);
+	if (!wait_event_timeout(card->param_wq, !skb_shared(skb), 5 * HZ))
+		dev_warn(&card->dev->dev,
+			 "Timeout waiting for VCC close on port %d\n", port);
+
+	dev_kfree_skb(skb);
 
 	/* Hold up vcc_destroy_socket() (our caller) until solos_bh() in the
 	   tasklet has finished processing any incoming packets (and, more to
@@ -990,10 +990,8 @@ static uint32_t fpga_tx(struct solos_card *card)
 				atomic_inc(&vcc->stats->tx);
 				solos_pop(vcc, oldskb);
 			} else {
-				struct pkt_hdr *header = (void *)oldskb->data;
-				if (le16_to_cpu(header->type) == PKT_PCLOSE)
-					complete(&SKB_CB(oldskb)->c);
 				dev_kfree_skb_irq(oldskb);
+				wake_up(&card->param_wq);
 			}
 		}
 	}

-- 
dwmw2


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6171 bytes --]

Re: [PATCH 00/17] ATM fixes for pppoatm/br2684

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1676 bytes --]

On Sat, 2012-12-01 at 17:33 +0000, David Woodhouse wrote:
> 
> Very glad I added the BUILD_BUG_ON on the cb struct size now. Perhaps
> there should be a generic helper for that? Something like
>  skb_cb_cast(struct foo_cb, skb) could do it automatically...?

Something like this, perhaps? Using skb_cast_cb() would then make it
fairly much impossible to accidentally overflow the size of the skb cb.

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index f2af494..b4cb2cf 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -567,6 +567,11 @@ static inline struct rtable *skb_rtable(const struct sk_buff *skb)
 	return (struct rtable *)skb_dst(skb);
 }
 
+#define skb_cast_cb(skb, type)  ({				\
+	BUILD_BUG_ON(sizeof(type) > sizeof((skb)->cb));		\
+	(type *)(skb)->cb;					\
+	})
+
 extern void kfree_skb(struct sk_buff *skb);
 extern void skb_tx_error(struct sk_buff *skb);
 extern void consume_skb(struct sk_buff *skb);
diff --git a/drivers/atm/solos-pci.c b/drivers/atm/solos-pci.c
index 6619a8a..8dccf82 100644
--- a/drivers/atm/solos-pci.c
+++ b/drivers/atm/solos-pci.c
@@ -97,7 +97,7 @@ struct solos_skb_cb {
 };
 
 
-#define SKB_CB(skb)		((struct solos_skb_cb *)skb->cb)
+#define SKB_CB(skb)	skb_cast_cb(skb, struct solos_skb_cb)
 
 #define PKT_DATA	0
 #define PKT_COMMAND	1
@@ -1324,8 +1324,6 @@ static struct pci_driver fpga_driver = {
 
 static int __init solos_pci_init(void)
 {
-	BUILD_BUG_ON(sizeof(struct solos_skb_cb) > sizeof(((struct sk_buff *)0)->cb));
-
 	printk(KERN_INFO "Solos PCI Driver Version %s\n", VERSION);
 	return pci_register_driver(&fpga_driver);
 }


-- 
dwmw2


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6171 bytes --]

Re: [PATCH 00/17] ATM fixes for pppoatm/br2684

[David Miller]

From: David Woodhouse <dwmw2@infradead.org>
Date: Sun, 02 Dec 2012 00:35:47 +0000

> On Sat, 2012-12-01 at 11:44 -0500, David Miller wrote:
>> 
>> > drivers/atm/solos-pci.c: In function ‘solos_pci_init’:
>> > drivers/atm/solos-pci.c:1329:2: error: size of unnamed array is
>> negative
>> 
>> It's from adding the completion to the solos skb cb, you can't do
>> that.  It won't fit on 64-bit when all debugging kconfig options are
>> enabled.
> 
> Thanks for catching that. I've just posted a [v2] version of the
> offending patch, which no longer puts a completion into the skb cb.
> 
> I'd appreciate a slightly more clueful eye looking over the incremental
> patch (below) just to confirm that the new method is correct, but it
> certainly seems to work. This version is identical to the one I posted
> earlier, except that I use dev_kfree_skb() in the pclose() function
> instead of dev_kfree_skb_any(). We know this will be called from a
> suitable context, and it even uses GFP_KERNEL a few lines higher up.
> 
> If that's OK, please pull the resulting tree from
> 	git://git.infradead.org/users/dwmw2/atm.git

Looks good, pulled, thanks David.

Re: [PATCH 00/17] ATM fixes for pppoatm/br2684

[David Miller]

From: David Woodhouse <dwmw2@infradead.org>
Date: Sun, 02 Dec 2012 00:40:47 +0000

> On Sat, 2012-12-01 at 17:33 +0000, David Woodhouse wrote:
>> 
>> Very glad I added the BUILD_BUG_ON on the cb struct size now. Perhaps
>> there should be a generic helper for that? Something like
>>  skb_cb_cast(struct foo_cb, skb) could do it automatically...?
> 
> Something like this, perhaps? Using skb_cast_cb() would then make it
> fairly much impossible to accidentally overflow the size of the skb cb.

I actually prefer what we do now, which is do the BUILD_BUG_ON()
once in the subsystem specific code, usually the initializer.

It's part of creating a new SKB cb, adding that assertion somewhere.

Re: [PATCH 00/17] ATM fixes for pppoatm/br2684

[Chas Williams (CONTRACTOR)]

In message <1354382493.21562.347.camel@shinybook.infradead.org>,David Woodhouse writes:
>Possibly not even a bug at all, in fact =E2=80=94 GCC is fairly loose with =
>those
>warnings. But if it is a bug it's my fault. I'll take a look at that
>too. Not tonight though; I'm going out shortly and will only just manage
>the solos-pci fix.

it is not a bug, just gcc being pedantic.  gcc doesn't believe
that passing by reference it a form of initilization.

Re: [PATCH 00/17] ATM fixes for pppoatm/br2684

[David Miller]

From: "Chas Williams (CONTRACTOR)" <chas@cmf.nrl.navy.mil>
Date: Sat, 01 Dec 2012 20:57:35 -0500

> In message <1354382493.21562.347.camel@shinybook.infradead.org>,David Woodhouse writes:
>>Possibly not even a bug at all, in fact =E2=80=94 GCC is fairly loose with =
>>those
>>warnings. But if it is a bug it's my fault. I'll take a look at that
>>too. Not tonight though; I'm going out shortly and will only just manage
>>the solos-pci fix.
> 
> it is not a bug, just gcc being pedantic.  gcc doesn't believe
> that passing by reference it a form of initilization.

It's not that, it actually is analyzing the initializations done by
that function call during inlining.

What it can't see is the case where multiple flows of control have
different treatments of a variable.

It can't see that, for example, all paths that test boolean X do not
touch the uninitialized variable.  It doesn't analyze the
inter-dependencies of each code path in enough detail to know for
certain.

Re: [PATCH 00/17] ATM fixes for pppoatm/br2684

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 803 bytes --]

On Sat, 2012-12-01 at 20:49 -0500, David Miller wrote:
> 
> I actually prefer what we do now, which is do the BUILD_BUG_ON()
> once in the subsystem specific code, usually the initializer.
> 
> It's part of creating a new SKB cb, adding that assertion somewhere.

Where it's *subsystem* code that's great... but *drivers* get to use the
skb cb too. And driver authors aren't always so reliable :)

Looking just at solos-pci, there was no feedback during the initial
submission that I ought to be using a BUILD_BUG_ON to limit the size of
struct solos_skb_cb. It was just luck, really, that I remembered to do
it — as an afterthought in a later iteration of the patch.

Giving driver authors fewer ways to shoot themselves in the foot always
seems like a good idea... 

-- 
dwmw2


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6171 bytes --]

Checking struct size against sizeof(skb->cb) (was Re: [PATCH 00/17] ATM fixes for pppoatm/br2684)

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 4031 bytes --]

On Sun, 2012-12-02 at 08:14 +0000, David Woodhouse wrote:
> On Sat, 2012-12-01 at 20:49 -0500, David Miller wrote:
> > 
> > I actually prefer what we do now, which is do the BUILD_BUG_ON()
> > once in the subsystem specific code, usually the initializer.
> > 
> > It's part of creating a new SKB cb, adding that assertion somewhere.
> 
> Where it's *subsystem* code that's great...

Hmm... or maybe not. A quick check suggests that about two-thirds of
users even in net/ aren't actually doing the check. My brief
investigation gives a score of 30 to 14 (or thereabouts; there may be
some that *do* check, but I failed to spot it. And I used a Fedora
config, not allyesconfig).

If you don't want an automatic check/cast macro (which is only adding a
compile-time check; no runtime overhead), then is it worth doing a
bombing run on the offenders listed below and adding the manual checks?
And I'll look at *drivers* next... which I suspect will be worse.

I concede there are probably no actual *bugs* being hidden here — I
don't think any of them actually *do* overflow. But since the check is
free at run-time, we *ought* to be doing it. Even if a given struct is
tiny and there's no *chance* of it overflowing, people might still add
to it. After all, the solos_skb_cb struct was tiny too until I stupidly
added a completion to it.

(Actually, I'm not entirely sure about 'no bugs'. The L2TP thing with
starting its own struct at &skb->cb[sizeof(struct inet_skb_parm)]
doesn't make it overflow, but what the hell is l2tp_xmit_skb() doing
poking at IPCB(skb) anyway... is it even guaranteed to be Legacy IP? Can
it not be IPv6? And why would anything else *trust* the contents of ->cb
on a skb that just got handed to it?)


My list:

Size not checked against sizeof(skb->cb):

struct napi_gro_cb (include/linux/netdevice.h)
struct ip6_mtuinfo (include/linux/ipv6.h)
struct sctp_ulpevent (include/net/sctp/ulpevent.h)
struct bt_skb_cb (include/net/bluetooth/bluetooth.h)
struct atm_skb_data (include/linux/atmdev.h)
struct br_input_skb_cb (net/bridge/br_private.h)
struct hci_cb (include/net/bluetooth/hci_core.h)
struct sock_exterr_skb (include/linux/errqueue.h)
struct udp_skb_cb (include/net/udp.h)
struct ipx_cb (include/net/ipx.h)
struct neighbour_cb (include/net/neighbour.h)
struct ip6frag_skb_cb (net/ipv6/reassembly.c)
struct dev_gso_cb (net/core/dev.c)
struct irda_skb_cb (include/net/irda/irda_device.h)
struct cmtp_skb (net/bluetooth/cmtp/cmtp.h)
struct ipfrag_skb_cb (net/ipv6/ip_fragment.c)
struct xfrm_skb_cb (include/net/xfrm.h)
struct xfrm_mode_skb_cb (include/net/xfrm.h)
struct xfrm_spi_skb_cb (include/net/xfrm.h)
struct in_pktinfo (include/uapi/linux/in.h)
struct nf_ct_frag6_skb_cb (net/ipv6/netfilter/nf_conntrack_reasm.c)
struct l2tp_skb_cb (net/l2tp/l2tp_core.c) (less than full cb)
struct ah_skb_cb (net/ipv4/ah4.c)
struct ah_skb_cb (net/ipv6/ah6.c)
struct esp_skb_cb (net/ipv4/esp4.c)
struct esp_skb_cb (net/ipv6/esp6.c)
struct skb_eosp_msg_data (net/mac80211/ieee80211_i.h)
struct mISDNhead (include/linux/mISDNif.h)
struct ieee80211_ra_tid (net/mac80211/iface.c)
struct sctp_input_cb (net/sctp/input.c)

Checked:

struct unix_skb_parms (include/net/af_unix.h)
struct packet_skb_cb (net/packet/af_packet.c)
struct ovs_skb_cb (net/openvswitch/datapath.h)
struct ieee80211_tx_info (include/net/mac80211.h)
struct ieee80211_rx_status (include/net/mac80211.h)
struct tcp_skb_cb (include/net/tcp.h)
struct ieee802154_mac_cb (include/net/ieee802154_netdev.h)
struct netlink_cb_parms (include/linux/netlink.h)
struct inet6_skb_parm (include/linux/ipv6.h)
struct inet_skb_parm (include/net/ip.h)
struct tcp_skb_cb (include/net/tcp.h)
struct garp_skb_cb (include/net/garp.h)
struct dccp_skb_cb (net/dccp/dccp.h)
struct qdisc_skb_cb (include/net/sch_generic.h)

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation




[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6171 bytes --]

skb->cb size checks (was Re: [PATCH 00/17] ATM fixes for pppoatm/br2684)

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1996 bytes --]

On Sat, 2012-12-01 at 20:49 -0500, David Miller wrote:
> From: David Woodhouse <dwmw2@infradead.org>
> Date: Sun, 02 Dec 2012 00:40:47 +0000
> 
> > On Sat, 2012-12-01 at 17:33 +0000, David Woodhouse wrote:
> >> 
> >> Very glad I added the BUILD_BUG_ON on the cb struct size now. Perhaps
> >> there should be a generic helper for that? Something like
> >>  skb_cb_cast(struct foo_cb, skb) could do it automatically...?
> > 
> > Something like this, perhaps? Using skb_cast_cb() would then make it
> > fairly much impossible to accidentally overflow the size of the skb cb.
> 
> I actually prefer what we do now, which is do the BUILD_BUG_ON()
> once in the subsystem specific code, usually the initializer.
> 
> It's part of creating a new SKB cb, adding that assertion somewhere.

I looked harder at this, and should follow up before it actually does
fall out of the cracks in my brain and get completely forgotten.

Basically, you lie :)

What we *actually* do now, in about two-thirds of cases¹ even in net/
code (I didn't even look at drivers, which I expect to be worse), is use
skb->cb without any form of automatic size check at all. No manual
BUILD_BUG_ON() or anything.

Admittedly, in almost all cases that *isn't* a real problem, because the
structure *isn't* too big for skb->cb and it's all fine. But as a matter
of principle we probably *should* be doing those checks. Just in *case*
someone comes along and adds something stupid to the structure.

So... should we:
 - Ignore the "problem" and leave things as they are.

 - Go through and fix the 2/3 of offending net/ code and then the
   drivers too, *without* making the generic 'deference and automatic
   check' macro that I think would simplify that and help to keep us
   honest in future.
or
 - Let me add something like the skb_cast_cb() macro I wanted, then use
   it in all the offending code I can find.

-- 
dwmw2

¹ http://www.spinics.net/lists/netdev/msg218642.html


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6171 bytes --]