[PATCH 2.6.20-rc2] [BUGFIX] drivers/atm/firestream.c: Fix infinite recursion when alignment passed is 0.

[PATCH 2.6.20-rc2] [BUGFIX] drivers/atm/firestream.c: Fix infinite recursion when alignment passed is 0.

[Amit Choudhary]

Description: Fix infinite recursion when alignment passed is 0 in function aligned_kmalloc(), in file drivers/atm/firestream.c. Also, a negative value for alignment does not make sense. Check for negative value too.
The function prototype is:
		static void __devinit *aligned_kmalloc (int size, gfp_t flags, int alignment).


Signed-off-by: Amit Choudhary <amit2030@gmail.com>

diff --git a/drivers/atm/firestream.c b/drivers/atm/firestream.c
index 9c67df5..2ba6b2e 100644
--- a/drivers/atm/firestream.c
+++ b/drivers/atm/firestream.c
@@ -1385,7 +1385,7 @@ static void __devinit *aligned_kmalloc (
 
 	if (alignment <= 0x10) {
 		t = kmalloc (size, flags);
-		if ((unsigned long)t & (alignment-1)) {
+		if ((unsigned long)t & ((alignment <= 0) ? 0 : (alignment-1))) {
 			printk ("Kmalloc doesn't align things correctly! %p\n", t);
 			kfree (t);
 			return aligned_kmalloc (size, flags, alignment * 4);

Re: [PATCH 2.6.20-rc2] [BUGFIX] drivers/atm/firestream.c: Fix infinite recursion when alignment passed is 0.

[Mitchell Blank Jr]

First, if you want to get patches merged you should send them to the subsystem
maintained (in this case Chas, who I've cc:'ed)  Also if you feel it needs to
be sent to mailing list you should usually use a more specific list first
(like the ATM list or maybe netdev)  Please see Documentation/SubmittingPatches

Amit Choudhary wrote:
> Description: Fix infinite recursion when alignment passed is 0 in function aligned_kmalloc(),

I'm curious how you hit this -- the only caller to aligned_kmalloc() passes
a constant "0x10"

Looking at aligned_kmalloc() it seems to be pretty badly broken -- its fallback
if it gets a non-aligned buffer is to just try a larger size which doesn't
necessarily fix the problem.  It looks like explicitly aligning the buffer
is a better solution.

Could you test this patch?  If it works feel free to forward it on to Chas.

-Mitch

[PATCH] [ATM] remove firestream.c's aligned_kmalloc()

Signed-off-by: Mitchell Blank Jr <mitch@sfgoth.com>

diff --git a/drivers/atm/firestream.c b/drivers/atm/firestream.c
index 9c67df5..df8b0c0 100644
--- a/drivers/atm/firestream.c
+++ b/drivers/atm/firestream.c
@@ -1379,38 +1379,22 @@ static void reset_chip (struct fs_dev *d
 	}
 }
 
-static void __devinit *aligned_kmalloc (int size, gfp_t flags, int alignment)
-{
-	void  *t;
-
-	if (alignment <= 0x10) {
-		t = kmalloc (size, flags);
-		if ((unsigned long)t & (alignment-1)) {
-			printk ("Kmalloc doesn't align things correctly! %p\n", t);
-			kfree (t);
-			return aligned_kmalloc (size, flags, alignment * 4);
-		}
-		return t;
-	}
-	printk (KERN_ERR "Request for > 0x10 alignment not yet implemented (hard!)\n");
-	return NULL;
-}
-
 static int __devinit init_q (struct fs_dev *dev, 
 			  struct queue *txq, int queue, int nentries, int is_rq)
 {
-	int sz = nentries * sizeof (struct FS_QENTRY);
+	unsigned sz = nentries * sizeof (struct FS_QENTRY);
 	struct FS_QENTRY *p;
+	void *vp;
 
 	func_enter ();
 
 	fs_dprintk (FS_DEBUG_INIT, "Inititing queue at %x: %d entries:\n", 
 		    queue, nentries);
-
-	p = aligned_kmalloc (sz, GFP_KERNEL, 0x10);
+	vp = kmalloc(sz + 0xF, GFP_KERNEL);
+	p = (struct FS_QENTRY *) ALIGN((unsigned long) vp, 0x10);
 	fs_dprintk (FS_DEBUG_ALLOC, "Alloc queue: %p(%d)\n", p, sz);
 
-	if (!p) return 0;
+	if (!vp) return 0;
 
 	write_fs (dev, Q_SA(queue), virt_to_bus(p));
 	write_fs (dev, Q_EA(queue), virt_to_bus(p+nentries-1));
@@ -1423,8 +1407,7 @@ static int __devinit init_q (struct fs_d
 		write_fs (dev, Q_CNF(queue), 0 ); 
 	}
 
-	txq->sa = p;
-	txq->ea = p;
+	txq->buf = vp;
 	txq->offset = queue; 
 
 	func_exit ();
@@ -1529,8 +1512,8 @@ static void __devexit free_queue (struct
 	write_fs (dev, Q_WP(txq->offset), 0);
 	/* Configuration ? */
 
-	fs_dprintk (FS_DEBUG_ALLOC, "Free queue: %p\n", txq->sa);
-	kfree (txq->sa);
+	fs_dprintk (FS_DEBUG_ALLOC, "Free queue: %p\n", txq->buf);
+	kfree (txq->buf);
 
 	func_exit ();
 }
diff --git a/drivers/atm/firestream.h b/drivers/atm/firestream.h
index 49e783e..5408766 100644
--- a/drivers/atm/firestream.h
+++ b/drivers/atm/firestream.h
@@ -455,7 +455,7 @@ struct fs_vcc {
 
 
 struct queue {
-	struct FS_QENTRY *sa, *ea;  
+	void *buf;  
 	int offset;
 };
 

Re: [PATCH 2.6.20-rc2] [BUGFIX] drivers/atm/firestream.c: Fix infinite recursion when alignment passed is 0.

[David Miller]

From: Amit Choudhary <amit2030@gmail.com>
Date: Sat, 30 Dec 2006 18:26:03 -0800

> Description: Fix infinite recursion when alignment passed is 0 in function aligned_kmalloc(), in file drivers/atm/firestream.c. Also, a negative value for alignment does not make sense. Check for negative value too.
> The function prototype is:
> 		static void __devinit *aligned_kmalloc (int size, gfp_t flags, int alignment).
> 
> 
> Signed-off-by: Amit Choudhary <amit2030@gmail.com>

There is only one call to aligned_kmalloc() and it passes in
0x10 as the alignment argument.  Therefore all of this checking
is completely pointless.

Re: [PATCH 2.6.20-rc2] [BUGFIX] drivers/atm/firestream.c: Fix infinite recursion when alignment passed is 0.

[Pekka Enberg]

On 12/31/06, Mitchell Blank Jr <mitch@sfgoth.com> wrote:
> Looking at aligned_kmalloc() it seems to be pretty badly broken -- its fallback
> if it gets a non-aligned buffer is to just try a larger size which doesn't
> necessarily fix the problem.  It looks like explicitly aligning the buffer
> is a better solution.

Shouldn't we be using dma_alloc_*() here instead of abusing kmalloc()?