Netdev List
 help / color / mirror / Atom feed
* Re: [PATCH] ircomm: release tty before sleeping potentially indefintely
From: Peter Hurley @ 2013-03-04  4:30 UTC (permalink / raw)
  To: David Miller; +Cc: sasha.levin, samuel, gregkh, jslaby, netdev, linux-kernel
In-Reply-To: <20130303.213601.1201168888595187293.davem@davemloft.net>


On Sun, 2013-03-03 at 21:36 -0500, David Miller wrote:
> From: Peter Hurley <peter@hurleysoftware.com>
> Date: Sun, 03 Mar 2013 20:06:18 -0500
> 
> > But regardless, this function __cannot__ sleep holding the tty_lock().
> 
> So drop it across the schedule(), but recheck the termios after
> regrabbing it.

I'll have to do some research on that.

1) The code is using a deliberate snapshot.

	if (tty->termios.c_cflag & CLOCAL) {
		IRDA_DEBUG(1, "%s(), doing CLOCAL!\n", __func__ );
		do_clocal = 1;
	}

	.....

	while (1) {

		.........

		/*
		 * Check if link is ready now. Even if CLOCAL is
		 * specified, we cannot return before the IrCOMM link is
		 * ready
		 */
		if (!test_bit(ASYNCB_CLOSING, &port->flags) &&
		    (do_clocal || tty_port_carrier_raised(port)) &&
		    self->state == IRCOMM_TTY_READY)
		{
			break;
		}


2) The only reason this driver isn't using tty_port_block_til_ready() is
the lone state check:
		    self->state == IRCOMM_TTY_READY

I take it IRDA has some kind of virtual cabling protocol. But it's
unclear why this can't be implemented in the driver without duplicating
tty_port_block_til_ready(). For example, if the device can't do CLOCAL
open (meaning no underlying device attached prior to open) then why
specify that in the driver flags? Additionally, CLOCAL can be masked out
by the driver's set_termios() method. And then it could implement the
state check in its .carrier_raised() method.

The net result of which would obviate the need for
ircomm_tty_block_til_ready() at all.

3) The do_clocal snapshot is universally employed by every tty driver. I
don't mean that as some kind of lame excuse. But if this should change,
it should change across every tty driver with a really good reason.

4) Rechecking termios will change the way the user-space open() for this
device behaves. And I need to think more on how that might or might not
be a problem.

5) The code behavior pre-dates the 2005 check-in so I'll probably have
to do some code archaeology.


That's probably going to take some time.

In the meantime, while reviewing that code, I noticed there's a handful
of serious bugs in that one function that I'll send a patchset for.

Plus, someone could be back to me on if and why the driver needs to be
virtually cabled to open().

Regards,
Peter Hurley

^ permalink raw reply

* Re: [PATCH] ircomm: release tty before sleeping potentially indefintely
From: Peter Hurley @ 2013-03-04  4:24 UTC (permalink / raw)
  To: David Miller; +Cc: sasha.levin, samuel, gregkh, jslaby, netdev, linux-kernel
In-Reply-To: <20130303.213601.1201168888595187293.davem@davemloft.net>

On Sun, 2013-03-03 at 21:36 -0500, David Miller wrote:
> From: Peter Hurley <peter@hurleysoftware.com>
> Date: Sun, 03 Mar 2013 20:06:18 -0500
> 
> > But regardless, this function __cannot__ sleep holding the tty_lock().
> 
> So drop it across the schedule(), but recheck the termios after
> regrabbing it.

I'll have to do some research on that.

1) The code is using a deliberate snapshot.

	if (tty->termios.c_cflag & CLOCAL) {
		IRDA_DEBUG(1, "%s(), doing CLOCAL!\n", __func__ );
		do_clocal = 1;
	}

	.....

	while (1) {

		.........

		/*
		 * Check if link is ready now. Even if CLOCAL is
		 * specified, we cannot return before the IrCOMM link is
		 * ready
		 */
		if (!test_bit(ASYNCB_CLOSING, &port->flags) &&
		    (do_clocal || tty_port_carrier_raised(port)) &&
		    self->state == IRCOMM_TTY_READY)
		{
			break;
		}


2) The only reason this driver isn't using tty_port_block_til_ready() is
the lone state check:
		    self->state == IRCOMM_TTY_READY

I take it IRDA has some kind of virtual cabling protocol. But it's
unclear why this can't be implemented in the driver without duplicating
tty_port_block_til_ready(). For example, if the device can't do CLOCAL
open (meaning no underlying device attached prior to open) then why
specify that in the driver flags? Additionally, CLOCAL can be masked out
by the driver's set_termios() method. And then it could implement the
state check in its .carrier_raised() method.

3) The do_clocal snapshot is universally employed by every tty driver. I
don't mean that as some kind of lame excuse. But if this should change,
it should change across every tty driver with a really good reason.

4) Rechecking termios will change the way the user-space open() for this
device behaves. And I need to think more on how that might or might not
be a problem.

5) The code behavior pre-dates the 2005 check-in so I'll probably have
to do some code archaeology.


That's probably going to take some time.

In the meantime, while reviewing that code, I noticed there's a handful
of serious bugs in that one function that I'll send a patchset for.

Plus, someone could be back to me on if and why the driver needs to be
virtually cabled to open().

Regards,
Peter Hurley

^ permalink raw reply

* Re: [RFC PATCH 1/5] net: implement support for low latency socket polling
From: Andi Kleen @ 2013-03-04  3:55 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: Andi Kleen, Eliezer Tamir, linux-kernel, netdev, Dave Miller,
	Jesse Brandeburg, e1000-devel, Willem de Bruijn, HPA,
	Eliezer Tamir
In-Reply-To: <1362345601.15793.83.camel@edumazet-glaptop>

On Sun, Mar 03, 2013 at 01:20:01PM -0800, Eric Dumazet wrote:
> On Sun, 2013-03-03 at 20:21 +0100, Andi Kleen wrote:
> > > Alternative to 2) would be to use a generation id, incremented every
> > > time a napi used in spin polling enabled driver is dismantled (and freed
> > > after RCU grace period)
> > > 
> > > And store in sockets not only the pointer to napi_struct, but the
> > > current generation id : If the generation id doesnt match, disable
> > > the spinpoll until next packet rebuilds the cache again.
> > 
> > This would require rcu_read_lock, aka preempt off, during polling, right?
> > 
> 
> Of course, polling probably needs BH disabling as well to get the per
> napi lock

Ok maybe the cond_resched() is good enough.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

^ permalink raw reply

* Re: [Patch net] sctp: use KMALLOC_MAX_SIZE instead of its own MAX_KMALLOC_SIZE
From: Neil Horman @ 2013-03-04  3:52 UTC (permalink / raw)
  To: Cong Wang
  Cc: netdev, linux-sctp, Vlad Yasevich, Sridhar Samudrala,
	David S. Miller
In-Reply-To: <1362364107-26908-1-git-send-email-amwang@redhat.com>

On Mon, Mar 04, 2013 at 10:28:27AM +0800, Cong Wang wrote:
> From: Cong Wang <amwang@redhat.com>
> 
> Don't definite its own MAX_KMALLOC_SIZE, use the one
> defined in mm.
> 
> Cc: Vlad Yasevich <vyasevich@gmail.com>
> Cc: Sridhar Samudrala <sri@us.ibm.com>
> Cc: Neil Horman <nhorman@tuxdriver.com>
> Cc: David S. Miller <davem@davemloft.net>
> Signed-off-by: Cong Wang <amwang@redhat.com>
> 
Acked-by: Neil Horman <nhorman@tuxdriver.com>

> ---
> diff --git a/net/sctp/ssnmap.c b/net/sctp/ssnmap.c
> index 442ad4e..825ea94 100644
> --- a/net/sctp/ssnmap.c
> +++ b/net/sctp/ssnmap.c
> @@ -41,8 +41,6 @@
>  #include <net/sctp/sctp.h>
>  #include <net/sctp/sm.h>
>  
> -#define MAX_KMALLOC_SIZE	131072
> -
>  static struct sctp_ssnmap *sctp_ssnmap_init(struct sctp_ssnmap *map, __u16 in,
>  					    __u16 out);
>  
> @@ -65,7 +63,7 @@ struct sctp_ssnmap *sctp_ssnmap_new(__u16 in, __u16 out,
>  	int size;
>  
>  	size = sctp_ssnmap_size(in, out);
> -	if (size <= MAX_KMALLOC_SIZE)
> +	if (size <= KMALLOC_MAX_SIZE)
>  		retval = kmalloc(size, gfp);
>  	else
>  		retval = (struct sctp_ssnmap *)
> @@ -82,7 +80,7 @@ struct sctp_ssnmap *sctp_ssnmap_new(__u16 in, __u16 out,
>  	return retval;
>  
>  fail_map:
> -	if (size <= MAX_KMALLOC_SIZE)
> +	if (size <= KMALLOC_MAX_SIZE)
>  		kfree(retval);
>  	else
>  		free_pages((unsigned long)retval, get_order(size));
> @@ -124,7 +122,7 @@ void sctp_ssnmap_free(struct sctp_ssnmap *map)
>  		int size;
>  
>  		size = sctp_ssnmap_size(map->in.len, map->out.len);
> -		if (size <= MAX_KMALLOC_SIZE)
> +		if (size <= KMALLOC_MAX_SIZE)
>  			kfree(map);
>  		else
>  			free_pages((unsigned long)map, get_order(size));
> 

^ permalink raw reply

* Re: [Patch net] garp: fix a NULL pointer dereference bug
From: Cong Wang @ 2013-03-04  3:41 UTC (permalink / raw)
  To: netdev; +Cc: bugs, David Ward, Stephen Hemminger, David S. Miller
In-Reply-To: <1362368217-30984-1-git-send-email-amwang@redhat.com>

On Mon, 2013-03-04 at 11:36 +0800, Cong Wang wrote:
> From: Cong Wang <amwang@redhat.com>
> 
> A NULL-deref bug was reported in:
> https://bugzilla.kernel.org/show_bug.cgi?id=54281
> when deleting a vlan interface:
> 
> # ip link  del em1.57
> BUG: unable to handle kernel NULL pointer dereference at (null)
> IP: [<ffffffffa03c356f>] garp_uninit_applicant+0x2f/0xd0 [garp]
> ...
> 
> This is probably app->pdu is NULL'ed in garp_pdu_rcv()
> in BH, while garp_uninit_applicant() only holds rtnl lock
> which is not enough to prevent this. 
> 
> OTOH, garp_pdu_queue() should ways be called with the spin lock.
> 
> 
> Reported-by: bugs@syam.in

I can't reproduce it locally, so please help to test it. I only did
compiling test.

Thanks!

^ permalink raw reply

* [Patch net] garp: fix a NULL pointer dereference bug
From: Cong Wang @ 2013-03-04  3:36 UTC (permalink / raw)
  To: netdev; +Cc: bugs, David Ward, Stephen Hemminger, David S. Miller, Cong Wang

From: Cong Wang <amwang@redhat.com>

A NULL-deref bug was reported in:
https://bugzilla.kernel.org/show_bug.cgi?id=54281
when deleting a vlan interface:

# ip link  del em1.57
BUG: unable to handle kernel NULL pointer dereference at (null)
IP: [<ffffffffa03c356f>] garp_uninit_applicant+0x2f/0xd0 [garp]
...

This is probably app->pdu is NULL'ed in garp_pdu_rcv()
in BH, while garp_uninit_applicant() only holds rtnl lock
which is not enough to prevent this. 

OTOH, garp_pdu_queue() should ways be called with the spin lock.


Reported-by: bugs@syam.in
Cc: bugs@syam.in
Cc: David Ward <david.ward@ll.mit.edu>
Cc: Stephen Hemminger <stephen@networkplumber.org>
Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Cong Wang <amwang@redhat.com>

---
diff --git a/net/802/garp.c b/net/802/garp.c
index 8456f5d..5d9630a 100644
--- a/net/802/garp.c
+++ b/net/802/garp.c
@@ -609,8 +609,12 @@ void garp_uninit_applicant(struct net_device *dev, struct garp_application *appl
 	/* Delete timer and generate a final TRANSMIT_PDU event to flush out
 	 * all pending messages before the applicant is gone. */
 	del_timer_sync(&app->join_timer);
+
+	spin_lock_bh(&app->lock);
 	garp_gid_event(app, GARP_EVENT_TRANSMIT_PDU);
 	garp_pdu_queue(app);
+	spin_unlock_bh(&app->lock);
+
 	garp_queue_xmit(app);
 
 	dev_mc_del(dev, appl->proto.group_address);

^ permalink raw reply related

* [PATCH v3 1/1 net-next] net: fec: put tx to napi poll function to fix dead lock
From: Frank Li @ 2013-03-04  3:34 UTC (permalink / raw)
  To: lznuaa, shawn.guo, B38611, davem, linux-arm-kernel, netdev
  Cc: s.hauer, Frank Li

up stack ndo_start_xmit already hold lock.
fec_enet_start_xmit needn't spin lock.
stat_xmit just update fep->cur_tx
fec_enet_tx just update fep->dirty_tx

Reserve a empty bdb to check full or empty
cur_tx == dirty_tx    means full
cur_tx == dirty_tx +1 means empty

So needn't is_full variable.

Fix spin lock deadlock
=================================
[ INFO: inconsistent lock state ]
3.8.0-rc5+ #107 Not tainted
---------------------------------
inconsistent {HARDIRQ-ON-W} -> {IN-HARDIRQ-W} usage.
ptp4l/615 [HC1[1]:SC0[0]:HE0:SE1] takes:
 (&(&list->lock)->rlock#3){?.-...}, at: [<8042c3c4>] skb_queue_tail+0x20/0x50
 {HARDIRQ-ON-W} state was registered at:
 [<80067250>] mark_lock+0x154/0x4e8
 [<800676f4>] mark_irqflags+0x110/0x1a4
 [<80069208>] __lock_acquire+0x494/0x9c0
 [<80069ce8>] lock_acquire+0x90/0xa4
 [<80527ad0>] _raw_spin_lock_bh+0x44/0x54
 [<804877e0>] first_packet_length+0x38/0x1f0
 [<804879e4>] udp_poll+0x4c/0x5c
 [<804231f8>] sock_poll+0x24/0x28
 [<800d27f0>] do_poll.isra.10+0x120/0x254
 [<800d36e4>] do_sys_poll+0x15c/0x1e8
 [<800d3828>] sys_poll+0x60/0xc8
 [<8000e780>] ret_fast_syscall+0x0/0x3c

 *** DEADLOCK ***

 1 lock held by ptp4l/615:
  #0:  (&(&fep->hw_lock)->rlock){-.-...}, at: [<80355f9c>] fec_enet_tx+0x24/0x268
  stack backtrace:
  Backtrace:
  [<800121e0>] (dump_backtrace+0x0/0x10c) from [<80516210>] (dump_stack+0x18/0x1c)
  r6:8063b1fc r5:bf38b2f8 r4:bf38b000 r3:bf38b000
  [<805161f8>] (dump_stack+0x0/0x1c) from [<805189d0>] (print_usage_bug.part.34+0x164/0x1a4)
  [<8051886c>] (print_usage_bug.part.34+0x0/0x1a4) from [<80518a88>] (print_usage_bug+0x78/0x88)
  r8:80065664 r7:bf38b2f8 r6:00000002 r5:00000000 r4:bf38b000
  [<80518a10>] (print_usage_bug+0x0/0x88) from [<80518b58>] (mark_lock_irq+0xc0/0x270)
  r7:bf38b000 r6:00000002 r5:bf38b2f8 r4:00000000
  [<80518a98>] (mark_lock_irq+0x0/0x270) from [<80067270>] (mark_lock+0x174/0x4e8)
  [<800670fc>] (mark_lock+0x0/0x4e8) from [<80067744>] (mark_irqflags+0x160/0x1a4)
  [<800675e4>] (mark_irqflags+0x0/0x1a4) from [<80069208>] (__lock_acquire+0x494/0x9c0)
  r5:00000002 r4:bf38b2f8
  [<80068d74>] (__lock_acquire+0x0/0x9c0) from [<80069ce8>] (lock_acquire+0x90/0xa4)
  [<80069c58>] (lock_acquire+0x0/0xa4) from [<805278d8>] (_raw_spin_lock_irqsave+0x4c/0x60)
  [<8052788c>] (_raw_spin_lock_irqsave+0x0/0x60) from [<8042c3c4>] (skb_queue_tail+0x20/0x50)
  r6:bfbb2180 r5:bf1d0190 r4:bf1d0184
  [<8042c3a4>] (skb_queue_tail+0x0/0x50) from [<8042c4cc>] (sock_queue_err_skb+0xd8/0x188)
  r6:00000056 r5:bfbb2180 r4:bf1d0000 r3:00000000
  [<8042c3f4>] (sock_queue_err_skb+0x0/0x188) from [<8042d15c>] (skb_tstamp_tx+0x70/0xa0)
  r6:bf0dddb0 r5:bf1d0000 r4:bfbb2180 r3:00000004
  [<8042d0ec>] (skb_tstamp_tx+0x0/0xa0) from [<803561d0>] (fec_enet_tx+0x258/0x268)
  r6:c089d260 r5:00001c00 r4:bfbd0000
  [<80355f78>] (fec_enet_tx+0x0/0x268) from [<803562cc>] (fec_enet_interrupt+0xec/0xf8)
  [<803561e0>] (fec_enet_interrupt+0x0/0xf8) from [<8007d5b0>] (handle_irq_event_percpu+0x54/0x1a0)
  [<8007d55c>] (handle_irq_event_percpu+0x0/0x1a0) from [<8007d740>] (handle_irq_event+0x44/0x64)
  [<8007d6fc>] (handle_irq_event+0x0/0x64) from [<80080690>] (handle_fasteoi_irq+0xc4/0x15c)
  r6:bf0dc000 r5:bf811290 r4:bf811240 r3:00000000
  [<800805cc>] (handle_fasteoi_irq+0x0/0x15c) from [<8007ceec>] (generic_handle_irq+0x28/0x38)
  r5:807130c8 r4:00000096
  [<8007cec4>] (generic_handle_irq+0x0/0x38) from [<8000f16c>] (handle_IRQ+0x54/0xb4)
  r4:8071d280 r3:00000180
  [<8000f118>] (handle_IRQ+0x0/0xb4) from [<80008544>] (gic_handle_irq+0x30/0x64)
  r8:8000e924 r7:f4000100 r6:bf0ddef8 r5:8071c974 r4:f400010c
  r3:00000000
  [<80008514>] (gic_handle_irq+0x0/0x64) from [<8000e2e4>] (__irq_svc+0x44/0x5c)
  Exception stack(0xbf0ddef8 to 0xbf0ddf40)

Signed-off-by: Frank Li <Frank.Li@freescale.com>
---
Change from v3 to v2
 * remove return value of fec_enet_tx
Change from v1 to v2
 * ignore TX package count in poll function

 drivers/net/ethernet/freescale/fec.c |   85 ++++++++++++++++-----------------
 drivers/net/ethernet/freescale/fec.h |    3 -
 2 files changed, 41 insertions(+), 47 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec.c b/drivers/net/ethernet/freescale/fec.c
index 0fe68c4..bca5a24 100644
--- a/drivers/net/ethernet/freescale/fec.c
+++ b/drivers/net/ethernet/freescale/fec.c
@@ -246,14 +246,13 @@ fec_enet_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 	struct bufdesc *bdp;
 	void *bufaddr;
 	unsigned short	status;
-	unsigned long flags;
+	unsigned int index;
 
 	if (!fep->link) {
 		/* Link is down or autonegotiation is in progress. */
 		return NETDEV_TX_BUSY;
 	}
 
-	spin_lock_irqsave(&fep->hw_lock, flags);
 	/* Fill in a Tx ring entry */
 	bdp = fep->cur_tx;
 
@@ -264,7 +263,6 @@ fec_enet_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 		 * This should not happen, since ndev->tbusy should be set.
 		 */
 		printk("%s: tx queue full!.\n", ndev->name);
-		spin_unlock_irqrestore(&fep->hw_lock, flags);
 		return NETDEV_TX_BUSY;
 	}
 
@@ -280,13 +278,13 @@ fec_enet_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 	 * 4-byte boundaries. Use bounce buffers to copy data
 	 * and get it aligned. Ugh.
 	 */
+	if (fep->bufdesc_ex)
+		index = (struct bufdesc_ex *)bdp -
+			(struct bufdesc_ex *)fep->tx_bd_base;
+	else
+		index = bdp - fep->tx_bd_base;
+
 	if (((unsigned long) bufaddr) & FEC_ALIGNMENT) {
-		unsigned int index;
-		if (fep->bufdesc_ex)
-			index = (struct bufdesc_ex *)bdp -
-				(struct bufdesc_ex *)fep->tx_bd_base;
-		else
-			index = bdp - fep->tx_bd_base;
 		memcpy(fep->tx_bounce[index], skb->data, skb->len);
 		bufaddr = fep->tx_bounce[index];
 	}
@@ -300,10 +298,7 @@ fec_enet_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 		swap_buffer(bufaddr, skb->len);
 
 	/* Save skb pointer */
-	fep->tx_skbuff[fep->skb_cur] = skb;
-
-	ndev->stats.tx_bytes += skb->len;
-	fep->skb_cur = (fep->skb_cur+1) & TX_RING_MOD_MASK;
+	fep->tx_skbuff[index] = skb;
 
 	/* Push the data cache so the CPM does not get stale memory
 	 * data.
@@ -331,26 +326,22 @@ fec_enet_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 			ebdp->cbd_esc = BD_ENET_TX_INT;
 		}
 	}
-	/* Trigger transmission start */
-	writel(0, fep->hwp + FEC_X_DES_ACTIVE);
-
 	/* If this was the last BD in the ring, start at the beginning again. */
 	if (status & BD_ENET_TX_WRAP)
 		bdp = fep->tx_bd_base;
 	else
 		bdp = fec_enet_get_nextdesc(bdp, fep->bufdesc_ex);
 
-	if (bdp == fep->dirty_tx) {
-		fep->tx_full = 1;
+	fep->cur_tx = bdp;
+
+	if (fep->cur_tx == fep->dirty_tx)
 		netif_stop_queue(ndev);
-	}
 
-	fep->cur_tx = bdp;
+	/* Trigger transmission start */
+	writel(0, fep->hwp + FEC_X_DES_ACTIVE);
 
 	skb_tx_timestamp(skb);
 
-	spin_unlock_irqrestore(&fep->hw_lock, flags);
-
 	return NETDEV_TX_OK;
 }
 
@@ -406,11 +397,8 @@ fec_restart(struct net_device *ndev, int duplex)
 		writel((unsigned long)fep->bd_dma + sizeof(struct bufdesc)
 			* RX_RING_SIZE,	fep->hwp + FEC_X_DES_START);
 
-	fep->dirty_tx = fep->cur_tx = fep->tx_bd_base;
 	fep->cur_rx = fep->rx_bd_base;
 
-	/* Reset SKB transmit buffers. */
-	fep->skb_cur = fep->skb_dirty = 0;
 	for (i = 0; i <= TX_RING_MOD_MASK; i++) {
 		if (fep->tx_skbuff[i]) {
 			dev_kfree_skb_any(fep->tx_skbuff[i]);
@@ -573,20 +561,35 @@ fec_enet_tx(struct net_device *ndev)
 	struct bufdesc *bdp;
 	unsigned short status;
 	struct	sk_buff	*skb;
+	int	index = 0;
 
 	fep = netdev_priv(ndev);
-	spin_lock(&fep->hw_lock);
 	bdp = fep->dirty_tx;
 
+	/* get next bdp of dirty_tx */
+	if (bdp->cbd_sc & BD_ENET_TX_WRAP)
+		bdp = fep->tx_bd_base;
+	else
+		bdp = fec_enet_get_nextdesc(bdp, fep->bufdesc_ex);
+
 	while (((status = bdp->cbd_sc) & BD_ENET_TX_READY) == 0) {
-		if (bdp == fep->cur_tx && fep->tx_full == 0)
+
+		/* current queue is empty */
+		if (bdp == fep->cur_tx)
 			break;
 
+		if (fep->bufdesc_ex)
+			index = (struct bufdesc_ex *)bdp -
+				(struct bufdesc_ex *)fep->tx_bd_base;
+		else
+			index = bdp - fep->tx_bd_base;
+
 		dma_unmap_single(&fep->pdev->dev, bdp->cbd_bufaddr,
 				FEC_ENET_TX_FRSIZE, DMA_TO_DEVICE);
 		bdp->cbd_bufaddr = 0;
 
-		skb = fep->tx_skbuff[fep->skb_dirty];
+		skb = fep->tx_skbuff[index];
+
 		/* Check for errors. */
 		if (status & (BD_ENET_TX_HB | BD_ENET_TX_LC |
 				   BD_ENET_TX_RL | BD_ENET_TX_UN |
@@ -631,8 +634,9 @@ fec_enet_tx(struct net_device *ndev)
 
 		/* Free the sk buffer associated with this last transmit */
 		dev_kfree_skb_any(skb);
-		fep->tx_skbuff[fep->skb_dirty] = NULL;
-		fep->skb_dirty = (fep->skb_dirty + 1) & TX_RING_MOD_MASK;
+		fep->tx_skbuff[index] = NULL;
+
+		fep->dirty_tx = bdp;
 
 		/* Update pointer to next buffer descriptor to be transmitted */
 		if (status & BD_ENET_TX_WRAP)
@@ -642,14 +646,12 @@ fec_enet_tx(struct net_device *ndev)
 
 		/* Since we have freed up a buffer, the ring is no longer full
 		 */
-		if (fep->tx_full) {
-			fep->tx_full = 0;
+		if (fep->dirty_tx != fep->cur_tx) {
 			if (netif_queue_stopped(ndev))
 				netif_wake_queue(ndev);
 		}
 	}
-	fep->dirty_tx = bdp;
-	spin_unlock(&fep->hw_lock);
+	return;
 }
 
 
@@ -816,7 +818,7 @@ fec_enet_interrupt(int irq, void *dev_id)
 		int_events = readl(fep->hwp + FEC_IEVENT);
 		writel(int_events, fep->hwp + FEC_IEVENT);
 
-		if (int_events & FEC_ENET_RXF) {
+		if (int_events & (FEC_ENET_RXF | FEC_ENET_TXF)) {
 			ret = IRQ_HANDLED;
 
 			/* Disable the RX interrupt */
@@ -827,15 +829,6 @@ fec_enet_interrupt(int irq, void *dev_id)
 			}
 		}
 
-		/* Transmit OK, or non-fatal error. Update the buffer
-		 * descriptors. FEC handles all errors, we just discover
-		 * them as part of the transmit process.
-		 */
-		if (int_events & FEC_ENET_TXF) {
-			ret = IRQ_HANDLED;
-			fec_enet_tx(ndev);
-		}
-
 		if (int_events & FEC_ENET_MII) {
 			ret = IRQ_HANDLED;
 			complete(&fep->mdio_done);
@@ -851,6 +844,8 @@ static int fec_enet_rx_napi(struct napi_struct *napi, int budget)
 	int pkts = fec_enet_rx(ndev, budget);
 	struct fec_enet_private *fep = netdev_priv(ndev);
 
+	fec_enet_tx(ndev);
+
 	if (pkts < budget) {
 		napi_complete(napi);
 		writel(FEC_DEFAULT_IMASK, fep->hwp + FEC_IMASK);
@@ -1646,6 +1641,7 @@ static int fec_enet_init(struct net_device *ndev)
 
 	/* ...and the same for transmit */
 	bdp = fep->tx_bd_base;
+	fep->cur_tx = bdp;
 	for (i = 0; i < TX_RING_SIZE; i++) {
 
 		/* Initialize the BD for every fragment in the page. */
@@ -1657,6 +1653,7 @@ static int fec_enet_init(struct net_device *ndev)
 	/* Set the last buffer to wrap */
 	bdp = fec_enet_get_prevdesc(bdp, fep->bufdesc_ex);
 	bdp->cbd_sc |= BD_SC_WRAP;
+	fep->dirty_tx = bdp;
 
 	fec_restart(ndev, 0);
 
diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
index 01579b8..c0f63be 100644
--- a/drivers/net/ethernet/freescale/fec.h
+++ b/drivers/net/ethernet/freescale/fec.h
@@ -214,8 +214,6 @@ struct fec_enet_private {
 	unsigned char *tx_bounce[TX_RING_SIZE];
 	struct	sk_buff *tx_skbuff[TX_RING_SIZE];
 	struct	sk_buff *rx_skbuff[RX_RING_SIZE];
-	ushort	skb_cur;
-	ushort	skb_dirty;
 
 	/* CPM dual port RAM relative addresses */
 	dma_addr_t	bd_dma;
@@ -227,7 +225,6 @@ struct fec_enet_private {
 	/* The ring entries to be free()ed */
 	struct bufdesc	*dirty_tx;
 
-	uint	tx_full;
 	/* hold while accessing the HW like ringbuffer for tx/rx but not MAC */
 	spinlock_t hw_lock;
 
-- 
1.7.1

^ permalink raw reply related

* Re: NET: FEC: dynamtic check DMA desc buff type
From: Greg Ungerer @ 2013-03-04  3:32 UTC (permalink / raw)
  To: Frank Li; +Cc: Sascha Hauer, netdev, lkml


Hi Frank,

Commit ff43da86c69d76a726ffe7d1666148960dc1d108 ("NET: FEC: dynamtic
check DMA desc buff type") breaks building of the fec ethernet driver
for non CONFIG_ARCH_MXC targets.

For example building for ColdFire devices with FEC hardware now
fails like this

drivers/net/ethernet/freescale/fec.c: In function ‘fec_enet_get_nextdesc’:
drivers/net/ethernet/freescale/fec.c:215:18: error: invalid use of undefined type ‘struct bufdesc_ex’
drivers/net/ethernet/freescale/fec.c: In function ‘fec_enet_get_prevdesc’:
drivers/net/ethernet/freescale/fec.c:224:18: error: invalid use of undefined type ‘struct bufdesc_ex’
drivers/net/ethernet/freescale/fec.c: In function ‘fec_enet_start_xmit’:
drivers/net/ethernet/freescale/fec.c:286:37: error: arithmetic on pointer to an incomplete type
drivers/net/ethernet/freescale/fec.c:287:13: error: arithmetic on pointer to an incomplete type
drivers/net/ethernet/freescale/fec.c:324:7: error: dereferencing pointer to incomplete type
etc....

These where generated on 3.9-rc1.

Can you please fix?

Regards
Greg

  

^ permalink raw reply

* Re: [PATCH] ircomm: release tty before sleeping potentially indefintely
From: David Miller @ 2013-03-04  2:36 UTC (permalink / raw)
  To: peter; +Cc: sasha.levin, samuel, gregkh, jslaby, netdev, linux-kernel
In-Reply-To: <1362359178.3221.118.camel@thor.lan>

From: Peter Hurley <peter@hurleysoftware.com>
Date: Sun, 03 Mar 2013 20:06:18 -0500

> But regardless, this function __cannot__ sleep holding the tty_lock().

So drop it across the schedule(), but recheck the termios after
regrabbing it.

^ permalink raw reply

* [Patch net] sctp: use KMALLOC_MAX_SIZE instead of its own MAX_KMALLOC_SIZE
From: Cong Wang @ 2013-03-04  2:28 UTC (permalink / raw)
  To: netdev
  Cc: linux-sctp, Vlad Yasevich, Sridhar Samudrala, Neil Horman,
	David S. Miller, Cong Wang

From: Cong Wang <amwang@redhat.com>

Don't definite its own MAX_KMALLOC_SIZE, use the one
defined in mm.

Cc: Vlad Yasevich <vyasevich@gmail.com>
Cc: Sridhar Samudrala <sri@us.ibm.com>
Cc: Neil Horman <nhorman@tuxdriver.com>
Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Cong Wang <amwang@redhat.com>

---
diff --git a/net/sctp/ssnmap.c b/net/sctp/ssnmap.c
index 442ad4e..825ea94 100644
--- a/net/sctp/ssnmap.c
+++ b/net/sctp/ssnmap.c
@@ -41,8 +41,6 @@
 #include <net/sctp/sctp.h>
 #include <net/sctp/sm.h>
 
-#define MAX_KMALLOC_SIZE	131072
-
 static struct sctp_ssnmap *sctp_ssnmap_init(struct sctp_ssnmap *map, __u16 in,
 					    __u16 out);
 
@@ -65,7 +63,7 @@ struct sctp_ssnmap *sctp_ssnmap_new(__u16 in, __u16 out,
 	int size;
 
 	size = sctp_ssnmap_size(in, out);
-	if (size <= MAX_KMALLOC_SIZE)
+	if (size <= KMALLOC_MAX_SIZE)
 		retval = kmalloc(size, gfp);
 	else
 		retval = (struct sctp_ssnmap *)
@@ -82,7 +80,7 @@ struct sctp_ssnmap *sctp_ssnmap_new(__u16 in, __u16 out,
 	return retval;
 
 fail_map:
-	if (size <= MAX_KMALLOC_SIZE)
+	if (size <= KMALLOC_MAX_SIZE)
 		kfree(retval);
 	else
 		free_pages((unsigned long)retval, get_order(size));
@@ -124,7 +122,7 @@ void sctp_ssnmap_free(struct sctp_ssnmap *map)
 		int size;
 
 		size = sctp_ssnmap_size(map->in.len, map->out.len);
-		if (size <= MAX_KMALLOC_SIZE)
+		if (size <= KMALLOC_MAX_SIZE)
 			kfree(map);
 		else
 			free_pages((unsigned long)map, get_order(size));

^ permalink raw reply related

* Re: [PATCH] ircomm: release tty before sleeping potentially indefintely
From: Peter Hurley @ 2013-03-04  2:23 UTC (permalink / raw)
  To: David Miller; +Cc: sasha.levin, samuel, gregkh, jslaby, netdev, linux-kernel
In-Reply-To: <20130303.174739.1195645942179862011.davem@davemloft.net>

On Sun, 2013-03-03 at 17:47 -0500, David Miller wrote:
> From: Sasha Levin <sasha.levin@oracle.com>
> Date: Sun,  3 Mar 2013 17:35:53 -0500
> 
> > ircomm_tty_block_til_ready would hold tty lock while blocking. Since the sleep
> > might take a long time we can prevent other processes from accessing the tty,
> > causing hung tasks and a dead tty.
> > 
> > Diagnosed-by: Peter Hurley <peter@hurleysoftware.com>
> > Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
> 
> But then you invalidate all of the tty state tests made under
> the lock at the beginning of this function, before enterring
> the loop.  If you drop the lock, those pieces of state could
> change.
> 
> I'm not applying this.

BTW, Sasha deserves a medal for finding and fixing this. Here's the
initial report [1] by him from Halloween. And he doesn't even have an IR
device.

So this fix needs to be cc'd to stable too.

Regards,
Peter Hurley


[1]

On Wed, 2012-10-31 at 16:10 -0400, Sasha Levin wrote:
On 10/31/2012 11:32 AM, Jiri Slaby wrote:
> > On 10/31/2012 04:30 PM, Sasha Levin wrote:
> >> On Wed, Oct 31, 2012 at 8:53 AM, Jiri Slaby <jslaby@suse.cz> wrote:
> >>> On 10/25/2012 08:02 PM, Sasha Levin wrote:
> >>>> Fuzzing with trinity inside a KVM tools (lkvm) guest with -next kernel
> >>>> uncovered the following warning:
> >>>
> >>> I cannot reproduce that :(. Do you still see it?
> >>
> >> Yes, it reproduces pretty easily while fuzzing.
> > 
> > What is your exact setup? I tried trinity with 100 000 syscalls inside
> > KVM with an LDEP-enabled kernel. How many serial ports do you have in
> > the guest? Any USB serials in there?
> 
> btw, I'm also seeing the following lockups, don't know if it's related:
> 
> 
> [ 2283.070569] INFO: task trinity-child20:9161 blocked for more than 120 seconds.
> [ 2283.071775] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
> [ 2283.074673] trinity-child20 D ffff8800276cb000  5424  9161   6364 0x00000000
> [ 2283.076018]  ffff880059d9da58 0000000000000002 0000000000000002 0000000000000000
> [ 2283.077393]  ffff880059d7b000 ffff880059d9dfd8 ffff880059d9dfd8 ffff880059d9dfd8
> [ 2283.078763]  ffff8800276cb000 ffff880059d7b000 ffff880059d9da78 ffff88001a095180
> [ 2283.084144] Call Trace:
> [ 2283.085039]  [<ffffffff83a98bd5>] schedule+0x55/0x60
> [ 2283.086748]  [<ffffffff83a98bf3>] schedule_preempt_disabled+0x13/0x20
> [ 2283.089000]  [<ffffffff83a9735d>] __mutex_lock_common+0x36d/0x5a0
> [ 2283.090658]  [<ffffffff83a9afb3>] ? tty_lock_nested+0x73/0x80
> [ 2283.091691]  [<ffffffff83a9afb3>] ? tty_lock_nested+0x73/0x80
> [ 2283.092779]  [<ffffffff83a975cf>] mutex_lock_nested+0x3f/0x50
> [ 2283.093875]  [<ffffffff83a9afb3>] tty_lock_nested+0x73/0x80
> [ 2283.094872]  [<ffffffff83a9afcb>] tty_lock+0xb/0x10
> [ 2283.095443]  [<ffffffff81bae880>] tty_open+0x270/0x5f0
> [ 2283.096181]  [<ffffffff8127cda8>] chrdev_open+0xf8/0x1d0
> [ 2283.097054]  [<ffffffff8127693c>] do_dentry_open+0x1fc/0x310
> [ 2283.098015]  [<ffffffff8127ccb0>] ? cdev_put+0x20/0x20
> [ 2283.098943]  [<ffffffff8127777a>] finish_open+0x4a/0x60
> [ 2283.099935]  [<ffffffff81286947>] do_last+0xb87/0xe70
> [ 2283.100910]  [<ffffffff812844b0>] ? link_path_walk+0x70/0x900
> [ 2283.101553]  [<ffffffff81286cf2>] path_openat+0xc2/0x500
> [ 2283.102282]  [<ffffffff83a9a314>] ? _raw_spin_unlock_irqrestore+0x84/0xb0
> [ 2283.103506]  [<ffffffff8128716c>] do_filp_open+0x3c/0xa0
> [ 2283.104282]  [<ffffffff81296c11>] ? __alloc_fd+0x1e1/0x200
> [ 2283.105278]  [<ffffffff81277c0c>] do_sys_open+0x11c/0x1c0
> [ 2283.106519]  [<ffffffff81277ccc>] sys_open+0x1c/0x20
> [ 2283.107241]  [<ffffffff81277d01>] sys_creat+0x11/0x20
> [ 2283.107975]  [<ffffffff83a9be18>] tracesys+0xe1/0xe6

^ permalink raw reply

* [Patch net] rds: limit the size allocated by rds_message_alloc()
From: Cong Wang @ 2013-03-04  2:18 UTC (permalink / raw)
  To: netdev
  Cc: rds-devel, Dave Jones, David S. Miller, Venkat Venkatsubra,
	Cong Wang

From: Cong Wang <amwang@redhat.com>

Dave Jones reported the following bug:

"When fed mangled socket data, rds will trust what userspace gives it,
and tries to allocate enormous amounts of memory larger than what
kmalloc can satisfy."

WARNING: at mm/page_alloc.c:2393 __alloc_pages_nodemask+0xa0d/0xbe0()
Hardware name: GA-MA78GM-S2H
Modules linked in: vmw_vsock_vmci_transport vmw_vmci vsock fuse bnep dlci bridge 8021q garp stp mrp binfmt_misc l2tp_ppp l2tp_core rfcomm s
Pid: 24652, comm: trinity-child2 Not tainted 3.8.0+ #65
Call Trace:
 [<ffffffff81044155>] warn_slowpath_common+0x75/0xa0
 [<ffffffff8104419a>] warn_slowpath_null+0x1a/0x20
 [<ffffffff811444ad>] __alloc_pages_nodemask+0xa0d/0xbe0
 [<ffffffff8100a196>] ? native_sched_clock+0x26/0x90
 [<ffffffff810b2128>] ? trace_hardirqs_off_caller+0x28/0xc0
 [<ffffffff810b21cd>] ? trace_hardirqs_off+0xd/0x10
 [<ffffffff811861f8>] alloc_pages_current+0xb8/0x180
 [<ffffffff8113eaaa>] __get_free_pages+0x2a/0x80
 [<ffffffff811934fe>] kmalloc_order_trace+0x3e/0x1a0
 [<ffffffff81193955>] __kmalloc+0x2f5/0x3a0
 [<ffffffff8104df0c>] ? local_bh_enable_ip+0x7c/0xf0
 [<ffffffffa0401ab3>] rds_message_alloc+0x23/0xb0 [rds]
 [<ffffffffa04043a1>] rds_sendmsg+0x2b1/0x990 [rds]
 [<ffffffff810b21cd>] ? trace_hardirqs_off+0xd/0x10
 [<ffffffff81564620>] sock_sendmsg+0xb0/0xe0
 [<ffffffff810b2052>] ? get_lock_stats+0x22/0x70
 [<ffffffff810b24be>] ? put_lock_stats.isra.23+0xe/0x40
 [<ffffffff81567f30>] sys_sendto+0x130/0x180
 [<ffffffff810b872d>] ? trace_hardirqs_on+0xd/0x10
 [<ffffffff816c547b>] ? _raw_spin_unlock_irq+0x3b/0x60
 [<ffffffff816cd767>] ? sysret_check+0x1b/0x56
 [<ffffffff810b8695>] ? trace_hardirqs_on_caller+0x115/0x1a0
 [<ffffffff81341d8e>] ? trace_hardirqs_on_thunk+0x3a/0x3f
 [<ffffffff816cd742>] system_call_fastpath+0x16/0x1b
---[ end trace eed6ae990d018c8b ]---

Reported-by: Dave Jones <davej@redhat.com>
Cc: Dave Jones <davej@redhat.com>
Cc: David S. Miller <davem@davemloft.net>
Cc: Venkat Venkatsubra <venkat.x.venkatsubra@oracle.com>
Signed-off-by: Cong Wang <amwang@redhat.com>

---
diff --git a/net/rds/message.c b/net/rds/message.c
index f0a4658..aff589c 100644
--- a/net/rds/message.c
+++ b/net/rds/message.c
@@ -197,6 +197,9 @@ struct rds_message *rds_message_alloc(unsigned int extra_len, gfp_t gfp)
 {
 	struct rds_message *rm;
 
+	if (extra_len > KMALLOC_MAX_SIZE - sizeof(struct rds_message))
+		return NULL;
+
 	rm = kzalloc(sizeof(struct rds_message) + extra_len, gfp);
 	if (!rm)
 		goto out;

^ permalink raw reply related

* Re: [PATCH] filter: fix sparse warning for bpf_internal_load_pointer_neg_helper
From: Eric Dumazet @ 2013-03-04  2:08 UTC (permalink / raw)
  To: Claudiu Ghioc
  Cc: davem, edumazet, dborkman, jpirko, akpm, netdev, linux-kernel,
	Claudiu Ghioc
In-Reply-To: <1362353404-27034-1-git-send-email-claudiu.ghioc@gmail.com>

On Mon, 2013-03-04 at 01:30 +0200, Claudiu Ghioc wrote:
> Removed the following sparse warning:
> *   net/core/filter.c:48:6: warning: symbol
>     'bpf_internal_load_pointer_neg_helper' was
>     not declared. Should it be static?
> 
> Signed-off-by: Claudiu Ghioc <claudiu.ghioc@gmail.com>
> ---
>  net/core/filter.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 2e20b55..f6c907c 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -45,7 +45,7 @@
>   *
>   * Exported for the bpf jit load helper.
>   */
> -void *bpf_internal_load_pointer_neg_helper(const struct sk_buff *skb, int k, unsigned int size)
> +static void *bpf_internal_load_pointer_neg_helper(const struct sk_buff *skb, int k, unsigned int size)
>  {
>  	u8 *ptr = NULL;
>  

Hmm, have you read the comment ?

Also :

^ permalink raw reply

* Re: [PATCH] ircomm: release tty before sleeping potentially indefintely
From: Peter Hurley @ 2013-03-04  1:06 UTC (permalink / raw)
  To: David Miller; +Cc: sasha.levin, samuel, gregkh, jslaby, netdev, linux-kernel
In-Reply-To: <20130303.193313.408641189137321065.davem@davemloft.net>

On Sun, 2013-03-03 at 19:33 -0500, David Miller wrote:
> From: Peter Hurley <peter@hurleysoftware.com>
> Date: Sun, 03 Mar 2013 19:04:25 -0500
> 
> > All these are re-tested in the loop. What state test isn't repeated?
> 
> One that rechecks the non-blocking filp flag, the
> TTY_IO_ERROR tty flag and the termios settings.
> 
> Like I said, all of the state tests performed at the beginning of
> this function, before enterring the loop.

How is O_NONBLOCK going to change? This function is sitting on the
user-space open.

The filp parameter is only on this task stack. It hasn't been linked in
anywhere else. Because of course the file isn't open yet because this
function hasn't returned success.

The TTY_IO_ERROR flag is used by drivers (this one included) to turn
away concurrent reads and writes when shutting down. The tty core does
not set this. Now this driver might set this, if commanded to hangup via
ircomm_tty_hangup, but like I said that's already handled in the loop
by testing tty_hung_up_p.

The initial termios setting cflag settings are set by the driver open().
In this driver, its here:

	driver->init_termios    = tty_std_termios;
	driver->init_termios.c_cflag = B9600 | CS8 | CREAD | HUPCL | CLOCAL;


Now, it's possible that one could construct an imaginary race, where the
tty has already been opened and that task now sets the termios without
CLOCAL and meanwhile a second task is racing this termios setting with
an open() of its own, but since there is no expectation from userspace
that those operations are serialized, there's no reason to serialize
them here.

But regardless, this function __cannot__ sleep holding the tty_lock().

Regards,
Peter Hurley

^ permalink raw reply

* Re: [PATCH] ircomm: release tty before sleeping potentially indefintely
From: David Miller @ 2013-03-04  0:33 UTC (permalink / raw)
  To: peter; +Cc: sasha.levin, samuel, gregkh, jslaby, netdev, linux-kernel
In-Reply-To: <1362355465.3221.82.camel@thor.lan>

From: Peter Hurley <peter@hurleysoftware.com>
Date: Sun, 03 Mar 2013 19:04:25 -0500

> All these are re-tested in the loop. What state test isn't repeated?

One that rechecks the non-blocking filp flag, the
TTY_IO_ERROR tty flag and the termios settings.

Like I said, all of the state tests performed at the beginning of
this function, before enterring the loop.

^ permalink raw reply

* Re: [PATCH] ircomm: release tty before sleeping potentially indefintely
From: David Miller @ 2013-03-04  0:31 UTC (permalink / raw)
  To: sasha.levin; +Cc: samuel, gregkh, jslaby, peter, netdev, linux-kernel
In-Reply-To: <5133DA12.8020907@oracle.com>

From: Sasha Levin <sasha.levin@oracle.com>
Date: Sun, 03 Mar 2013 18:17:38 -0500

> On 03/03/2013 05:47 PM, David Miller wrote:
>> From: Sasha Levin <sasha.levin@oracle.com>
>> Date: Sun,  3 Mar 2013 17:35:53 -0500
>> 
>>> ircomm_tty_block_til_ready would hold tty lock while blocking. Since the sleep
>>> might take a long time we can prevent other processes from accessing the tty,
>>> causing hung tasks and a dead tty.
>>>
>>> Diagnosed-by: Peter Hurley <peter@hurleysoftware.com>
>>> Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
>> 
>> But then you invalidate all of the tty state tests made under
>> the lock at the beginning of this function, before enterring
>> the loop.  If you drop the lock, those pieces of state could
>> change.
>> 
>> I'm not applying this.
> 
> I'm unsure. A similar patch was applied back in 2010 that does the same thing
> to a bunch of drivers, including the core tty code (e142a31da "tty: release
> BTM while sleeping in block_til_ready").
> 
> This IR code looks very much like tty_port_block_til_ready() where it was
> okay to do that change, so I should be the same with ircomm_tty_block_til_ready.

That assumes that the other changes don't have the same bug.

Releasing locks are dangerous, because it invalidates the context in
which all previous tests of state have been performed.  Anything can
happen to the TTY once you drop that lock.

^ permalink raw reply

* Re: [PATCH] ircomm: release tty before sleeping potentially indefintely
From: Peter Hurley @ 2013-03-04  0:04 UTC (permalink / raw)
  To: David Miller; +Cc: sasha.levin, samuel, gregkh, jslaby, netdev, linux-kernel
In-Reply-To: <20130303.174739.1195645942179862011.davem@davemloft.net>

On Sun, 2013-03-03 at 17:47 -0500, David Miller wrote:
> From: Sasha Levin <sasha.levin@oracle.com>
> Date: Sun,  3 Mar 2013 17:35:53 -0500
> 
> > ircomm_tty_block_til_ready would hold tty lock while blocking. Since the sleep
> > might take a long time we can prevent other processes from accessing the tty,
> > causing hung tasks and a dead tty.
> > 
> > Diagnosed-by: Peter Hurley <peter@hurleysoftware.com>
> > Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
> 
> But then you invalidate all of the tty state tests made under
> the lock at the beginning of this function, before enterring
> the loop.  If you drop the lock, those pieces of state could
> change.

Yes, the state could change. For example, the tty could be hung up while
ircomm_tty_block_til_ready() is sleeping. Or the session leader could be
exiting and SIGHUPed this task. Or the port could have been shutdown.

All these are re-tested in the loop. What state test isn't repeated?

> I'm not applying this.

That's certainly your perogative.
But you should know this bug hangs the entire tty subsystem.

This is the correct fix and exactly how this is done by the tty port.

Regards,
Peter Hurley

^ permalink raw reply

* Re: e1000e disabling already-disabled device
From: Francois Romieu @ 2013-03-03 23:36 UTC (permalink / raw)
  To: Dave Jones; +Cc: netdev, Bruce Allan
In-Reply-To: <20130303162124.GA31371@redhat.com>

Dave Jones <davej@redhat.com> :
[...]
>  ------------[ cut here ]------------
>  WARNING: at drivers/pci/pci.c:1397 pci_disable_device+0x90/0xa0()
>  Hardware name: 2356JK8
>  Device e1000e
> disabling already-disabled device
> Modules linked in:
>  dm_crypt arc4 iwldvm mac80211 ipv6 iTCO_wdt xfs iTCO_vendor_support libcrc32c coretemp kvm_intel kvm crc32c_intel ghash_clmulni_intel microcode snd_hda_codec_hdmi snd_hda_codec_realtek btusb bluetooth iwlwifi pcspkr snd_hda_intel cfg80211 snd_hda_codec snd_pcm lpc_ich mfd_core e1000e snd_page_alloc ptp pps_core thinkpad_acpi hwmon led_class rfkill wmi nfsd auth_rpcgss nfs_acl lockd sunrpc i915 i2c_algo_bit drm_kms_helper drm i2c_core video
> Pid: 19776, comm: kworker/u:11 Not tainted 3.8.0+ #6
> Call Trace:
>  [<ffffffff81049855>] warn_slowpath_common+0x75/0xa0
>  [<ffffffff81049936>] warn_slowpath_fmt+0x46/0x50
>  [<ffffffff81359760>] pci_disable_device+0x90/0xa0
>  [<ffffffffa0251092>] __e1000_shutdown+0x262/0x8a0 [e1000e]
>  [<ffffffffa0251743>] e1000_suspend+0x23/0x50 [e1000e]
>  [<ffffffff816284b9>] ? sub_preempt_count+0x79/0xd0
>  [<ffffffff8135bd97>] pci_pm_suspend+0x77/0x140
>  [<ffffffff8135bd20>] ? pci_pm_poweroff+0xf0/0xf0
>  [<ffffffff8140c678>] dpm_run_callback+0x58/0x90
>  [<ffffffff8140c79d>] __device_suspend+0xed/0x280
>  [<ffffffff8140d1ef>] async_suspend+0x1f/0xa0

Could it be that runtime power management is also enabled, you have no
link and thus end running both runtime_suspend and suspend at different
times ?

I wonder why the driver disables the PCI device here btw.

diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index a177b8b..5c643c6 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -5986,8 +5986,6 @@ static int __e1000_shutdown(struct pci_dev *pdev, bool *enable_wake,
 	 */
 	e1000e_release_hw_control(adapter);
 
-	pci_disable_device(pdev);
-
 	return 0;
 }
 
--- 
Ueimor

^ permalink raw reply related

* [PATCH] filter: fix sparse warning for bpf_internal_load_pointer_neg_helper
From: Claudiu Ghioc @ 2013-03-03 23:30 UTC (permalink / raw)
  To: davem; +Cc: edumazet, dborkman, jpirko, akpm, netdev, linux-kernel,
	Claudiu Ghioc

Removed the following sparse warning:
*   net/core/filter.c:48:6: warning: symbol
    'bpf_internal_load_pointer_neg_helper' was
    not declared. Should it be static?

Signed-off-by: Claudiu Ghioc <claudiu.ghioc@gmail.com>
---
 net/core/filter.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/filter.c b/net/core/filter.c
index 2e20b55..f6c907c 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -45,7 +45,7 @@
  *
  * Exported for the bpf jit load helper.
  */
-void *bpf_internal_load_pointer_neg_helper(const struct sk_buff *skb, int k, unsigned int size)
+static void *bpf_internal_load_pointer_neg_helper(const struct sk_buff *skb, int k, unsigned int size)
 {
 	u8 *ptr = NULL;
 
-- 
1.7.10.4

^ permalink raw reply related

* Re: [PATCH] ircomm: release tty before sleeping potentially indefintely
From: Sasha Levin @ 2013-03-03 23:17 UTC (permalink / raw)
  To: David Miller; +Cc: samuel, gregkh, jslaby, peter, netdev, linux-kernel
In-Reply-To: <20130303.174739.1195645942179862011.davem@davemloft.net>

On 03/03/2013 05:47 PM, David Miller wrote:
> From: Sasha Levin <sasha.levin@oracle.com>
> Date: Sun,  3 Mar 2013 17:35:53 -0500
> 
>> ircomm_tty_block_til_ready would hold tty lock while blocking. Since the sleep
>> might take a long time we can prevent other processes from accessing the tty,
>> causing hung tasks and a dead tty.
>>
>> Diagnosed-by: Peter Hurley <peter@hurleysoftware.com>
>> Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
> 
> But then you invalidate all of the tty state tests made under
> the lock at the beginning of this function, before enterring
> the loop.  If you drop the lock, those pieces of state could
> change.
> 
> I'm not applying this.

I'm unsure. A similar patch was applied back in 2010 that does the same thing
to a bunch of drivers, including the core tty code (e142a31da "tty: release
BTM while sleeping in block_til_ready").

This IR code looks very much like tty_port_block_til_ready() where it was
okay to do that change, so I should be the same with ircomm_tty_block_til_ready.


Thanks,
Sasha

^ permalink raw reply

* Re: [PATCH] CAIF: fix sparse warning for caif_usb
From: David Miller @ 2013-03-03 22:49 UTC (permalink / raw)
  To: silviupopescu1990; +Cc: netdev, sjur.brandeland, linux-kernel
In-Reply-To: <1362316188-2927-1-git-send-email-silviupopescu1990@gmail.com>

From: Silviu-Mihai Popescu <silviupopescu1990@gmail.com>
Date: Sun,  3 Mar 2013 15:09:48 +0200

> This fixes the following sparse warning:
> net/caif/caif_usb.c:84:16: warning: symbol 'cfusbl_create' was not
> declared. Should it be static?
> 
> Signed-off-by: Silviu-Mihai Popescu <silviupopescu1990@gmail.com>
> ---
>  net/caif/caif_usb.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/caif/caif_usb.c b/net/caif/caif_usb.c
> index 3ebc8cb..cbae9b0 100644
> --- a/net/caif/caif_usb.c
> +++ b/net/caif/caif_usb.c
> @@ -81,7 +81,7 @@ static void cfusbl_ctrlcmd(struct cflayer *layr, enum caif_ctrlcmd ctrl,
>  		layr->up->ctrlcmd(layr->up, ctrl, layr->id);
>  }
>  
> -struct cflayer *cfusbl_create(int phyid, u8 ethaddr[ETH_ALEN],
> +static struct cflayer *cfusbl_create(int phyid, u8 ethaddr[ETH_ALEN],
>  					u8 braddr[ETH_ALEN])

You need to fix up the indentation of the second line of
function arguments if you move the location of the openning
parenthesis on the first line, because all arguments on the
second and subsequent line must line up exactly at the first
column after the openning parenthesis of the first line.

Thanks.

^ permalink raw reply

* [ANNOUNCE] ulogd 2.0.2 release
From: Eric Leblond @ 2013-03-03 22:49 UTC (permalink / raw)
  To: netfilter-devel; +Cc: netdev, netfilter, netfilter-announce, lwn


[-- Attachment #1.1: Type: text/plain, Size: 572 bytes --]

Hi!

The Netfilter project proudly presents:

        ulogd 2.0.2

ulogd is a userspace logging daemon for netfilter/iptables related
logging. This includes per-packet logging of security violations,
per-packet logging for accounting, per-flow logging and flexible
user-defined accounting.

See ChangeLog that comes attached to this email for more details.

You can download it from:

http://www.netfilter.org/projects/ulogd/downloads.html
ftp://ftp.netfilter.org/pub/ulogd/

Have fun!
-- 
Eric Leblond <eric@regit.org>
Blog: https://home.regit.org/

[-- Attachment #1.2: changes-ulogd-2.0.2.txt --]
[-- Type: text/plain, Size: 1831 bytes --]

Bob Hockney (3):
      Fix parsing of ipv6 flowlabel and tc fields
      Handle postgresql schemas correctly
      Add additional ip6 header fields to database scripts

Eric Leblond (32):
      configure: add flag to disable NFACCT build
      configure: add flag to disable NFCT build
      configure: add flag to disable NFLOG build
      configure: display info about build plugins
      Use PRIu64 to print unsigned 64bit int
      XML: add missing include
      Add missing u64 function.
      Add GRAPHITE output module.
      nfct: call register callback on opened handler
      addr: add file containing addr utility functions.
      nfct: implement src and dst filter
      nfct: add protocol filter
      addr: fix compilation warning
      ulogd: add -v option to display message on stderr.
      conf: add flag to allow option setup tuning
      Add -l option to set log level from command line
      Get rid of SVN tag in comment.
      Fix warning on format
      base: fix warning on pointer handling
      graphite: fix warning about gnu extension usage
      Add TAGS to .gitignore
      Fix make distcheck
      graphite: fix crash on i386
      Update TODO.
      Suppress dead FIXME.
      Use access to ensure readability of config gile
      Fix typo in comments.
      Add handling of too long line and arguments.
      logemu: return error if configuration is invalid.
      Update man page.
      Update README
      Prepare release number to 2.0.2

Gustavo Zacarias (1):
      autoconf/ulogd: fix broken libpcap AM_CONDITIONAL

Jan Engelhardt (4):
      build: resolve automake-1.12 warnings
      build: resolve compile error due to missing nfacct CPPFLAGS
      build: avoid per-target CFLAGS
      build: move remaining preprocessor flags into CPPFLAGS


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

^ permalink raw reply

* Re: [PATCH] ircomm: release tty before sleeping potentially indefintely
From: David Miller @ 2013-03-03 22:47 UTC (permalink / raw)
  To: sasha.levin; +Cc: samuel, gregkh, jslaby, peter, netdev, linux-kernel
In-Reply-To: <1362350153-26225-1-git-send-email-sasha.levin@oracle.com>

From: Sasha Levin <sasha.levin@oracle.com>
Date: Sun,  3 Mar 2013 17:35:53 -0500

> ircomm_tty_block_til_ready would hold tty lock while blocking. Since the sleep
> might take a long time we can prevent other processes from accessing the tty,
> causing hung tasks and a dead tty.
> 
> Diagnosed-by: Peter Hurley <peter@hurleysoftware.com>
> Signed-off-by: Sasha Levin <sasha.levin@oracle.com>

But then you invalidate all of the tty state tests made under
the lock at the beginning of this function, before enterring
the loop.  If you drop the lock, those pieces of state could
change.

I'm not applying this.

^ permalink raw reply

* [PATCH] ircomm: release tty before sleeping potentially indefintely
From: Sasha Levin @ 2013-03-03 22:35 UTC (permalink / raw)
  To: samuel; +Cc: davem, gregkh, jslaby, peter, netdev, linux-kernel, Sasha Levin

ircomm_tty_block_til_ready would hold tty lock while blocking. Since the sleep
might take a long time we can prevent other processes from accessing the tty,
causing hung tasks and a dead tty.

Diagnosed-by: Peter Hurley <peter@hurleysoftware.com>
Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
---
 net/irda/ircomm/ircomm_tty.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/irda/ircomm/ircomm_tty.c b/net/irda/ircomm/ircomm_tty.c
index 9a5fd3c..7844cb3 100644
--- a/net/irda/ircomm/ircomm_tty.c
+++ b/net/irda/ircomm/ircomm_tty.c
@@ -355,7 +355,9 @@ static int ircomm_tty_block_til_ready(struct ircomm_tty_cb *self,
 		IRDA_DEBUG(1, "%s(%d):block_til_ready blocking on %s open_count=%d\n",
 		      __FILE__, __LINE__, tty->driver->name, port->count);
 
+		tty_unlock(tty);
 		schedule();
+		tty_lock(tty);
 	}
 
 	__set_current_state(TASK_RUNNING);
-- 
1.8.1.4

^ permalink raw reply related

* [ANNOUNCE] iptables 1.4.18 release
From: Pablo Neira Ayuso @ 2013-03-03 22:20 UTC (permalink / raw)
  To: netfilter-devel; +Cc: netdev, netfilter, netfilter-announce, lwn

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

Hi!

The Netfilter project proudly presents:

        iptables 1.4.18

iptables is the userspace command line program used to configure the
Linux 2.4.x and later packet filtering ruleset. It is targeted towards
system administrators.

See ChangeLog that comes attached to this email for more details.

You can download it from:

http://www.netfilter.org/projects/iptables/downloads.html
ftp://ftp.netfilter.org/pub/iptables/

Have fun!


[-- Attachment #2: changes-iptables-1.4.18.txt --]
[-- Type: text/plain, Size: 1350 bytes --]

Florian Westphal (1):
      doc: rpfilter: invert option should have own paragraph

Jan Engelhardt (11):
      build: resolve link failure for ip6t_NETMAP
      doc: fixup omissions in ip6tables-restore.8
      doc: document iptables-restore's -t option
      doc: document iptables-restore's -v option
      doc: document iptables-restore's -M option
      doc: document iptables-restore's -h option
      doc: name the supported log levels for ipt_LOG
      doc: mention -m in the manpage
      doc: document the -4 and -6 options
      extensions: S/DNPT: add missing save function
      build: bump SONAME for libxtables

Jozsef Kadlecsik (3):
      Introduce match/target aliases
      Add the "state" alias to the "conntrack" match
      Merge branch 'master' of vishnu.netfilter.org:/data/git/iptables

Pablo Neira Ayuso (7):
      iptables: remove unused leftover definitions
      libxtables: add xtables_rule_matches_free
      libxtables: add xtables_print_num
      Merge branch 'stable' into 'master'
      doc: document nat table for IPv6
      doc: iptables provides up to 5 independent tables
      build: bump version to 1.4.18

Ulrich Weber (3):
      extensions: libip6t_DNPT: fix wording in DNPT target
      extension: libip6t_DNAT: allow port DNAT without address
      extensions: libip6t_DNAT: set IPv6 DNAT --to-destination


^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox