[PATCH 2.6.30-rc5] ehea: fix invalid pointer access

[PATCH 2.6.30-rc5] ehea: fix invalid pointer access

[Hannes Hering]

This patch fixes an invalid pointer access in case the receive queue holds no
pointer to the next skb when the queue is empty.

Signed-off-by: Hannes Hering <hering2@de.ibm.com>
Signed-off-by: Jan-Bernd Themann <themann@de.ibm.com>
---

diff -Nurp -X dontdiff linux-2.6.30-rc4/drivers/net/ehea/ehea.h patched_kernel/drivers/net/ehea/ehea.h
--- linux-2.6.30-rc4/drivers/net/ehea/ehea.h	2009-05-04 09:54:42.000000000 +0200
+++ patched_kernel/drivers/net/ehea/ehea.h	2009-05-04 10:40:11.000000000 +0200
@@ -40,7 +40,7 @@
 #include <asm/io.h>
 
 #define DRV_NAME	"ehea"
-#define DRV_VERSION	"EHEA_0100"
+#define DRV_VERSION	"EHEA_0101"
 
 /* eHEA capability flags */
 #define DLPAR_PORT_ADD_REM 1
diff -Nurp -X dontdiff linux-2.6.30-rc4/drivers/net/ehea/ehea_main.c patched_kernel/drivers/net/ehea/ehea_main.c
--- linux-2.6.30-rc4/drivers/net/ehea/ehea_main.c	2009-05-04 09:54:42.000000000 +0200
+++ patched_kernel/drivers/net/ehea/ehea_main.c	2009-05-04 10:40:11.000000000 +0200
@@ -545,14 +545,17 @@ static inline struct sk_buff *get_skb_by
 	x &= (arr_len - 1);
 
 	pref = skb_array[x];
-	prefetchw(pref);
-	prefetchw(pref + EHEA_CACHE_LINE);
+	if (pref) {
+		prefetchw(pref);
+		prefetchw(pref + EHEA_CACHE_LINE);
+
+		pref = (skb_array[x]->data);
+		prefetch(pref);
+		prefetch(pref + EHEA_CACHE_LINE);
+		prefetch(pref + EHEA_CACHE_LINE * 2);
+		prefetch(pref + EHEA_CACHE_LINE * 3);
+	}
 
-	pref = (skb_array[x]->data);
-	prefetch(pref);
-	prefetch(pref + EHEA_CACHE_LINE);
-	prefetch(pref + EHEA_CACHE_LINE * 2);
-	prefetch(pref + EHEA_CACHE_LINE * 3);
 	skb = skb_array[skb_index];
 	skb_array[skb_index] = NULL;
 	return skb;
@@ -569,12 +572,14 @@ static inline struct sk_buff *get_skb_by
 	x &= (arr_len - 1);
 
 	pref = skb_array[x];
-	prefetchw(pref);
-	prefetchw(pref + EHEA_CACHE_LINE);
-
-	pref = (skb_array[x]->data);
-	prefetchw(pref);
-	prefetchw(pref + EHEA_CACHE_LINE);
+	if (pref) {
+		prefetchw(pref);
+		prefetchw(pref + EHEA_CACHE_LINE);
+
+		pref = (skb_array[x]->data);
+		prefetchw(pref);
+		prefetchw(pref + EHEA_CACHE_LINE);
+	}
 
 	skb = skb_array[wqe_index];
 	skb_array[wqe_index] = NULL;

Re: [PATCH 2.6.30-rc5] ehea: fix invalid pointer access

[David Miller]

From: Hannes Hering <hannes.hering@linux.vnet.ibm.com>
Date: Mon, 4 May 2009 18:02:30 +0200

> This patch fixes an invalid pointer access in case the receive queue holds no
> pointer to the next skb when the queue is empty.
> 
> Signed-off-by: Hannes Hering <hering2@de.ibm.com>
> Signed-off-by: Jan-Bernd Themann <themann@de.ibm.com>

Applied, thanks.

Re: [PATCH 2.6.30-rc5] ehea: fix invalid pointer access

[David Howells]

Hannes Hering <hannes.hering@linux.vnet.ibm.com> wrote:

>  	pref = skb_array[x];
> -	prefetchw(pref);
> -	prefetchw(pref + EHEA_CACHE_LINE);
> +	if (pref) {
> +		prefetchw(pref);
> +		prefetchw(pref + EHEA_CACHE_LINE);

Ummm...  Is prefetch() or prefetchw() faulting?

David

Re: [PATCH 2.6.30-rc5] ehea: fix invalid pointer access

[Hannes Hering]

On Tuesday 05 May 2009 11:11:27 David Howells wrote:
> Hannes Hering <hannes.hering@linux.vnet.ibm.com> wrote:
> 
> >  	pref = skb_array[x];
> > -	prefetchw(pref);
> > -	prefetchw(pref + EHEA_CACHE_LINE);
> > +	if (pref) {
> > +		prefetchw(pref);
> > +		prefetchw(pref + EHEA_CACHE_LINE);
> 
> Ummm...  Is prefetch() or prefetchw() faulting?
> 
> David
Hi David,

this is an ehea driver problem, which is occuring when the receive queue runs
empty. The faulting code is more specifically the following line:

	pref = (skb_array[x]->data);

Here the access to the struct element data would cause an exception.We could
have made the if block a little smaller.

Regards

Hannes

Re: [PATCH 2.6.30-rc5] ehea: fix invalid pointer access

[David Howells]

Hannes Hering <hannes.hering@linux.vnet.ibm.com> wrote:

> this is an ehea driver problem, which is occuring when the receive queue runs
> empty. The faulting code is more specifically the following line:
> 
> 	pref = (skb_array[x]->data);

In that case, you might want to move the prefetchw() calls in the following:

		pref = skb_array[x];
	-	prefetchw(pref);
	-	prefetchw(pref + EHEA_CACHE_LINE);
	+	if (pref) {
	+		prefetchw(pref);
	+		prefetchw(pref + EHEA_CACHE_LINE);

to before the if-statement.  That way the CPU can be attempting the prefetch
whilst it's chewing over the test and branch.  prefetching shouldn't fault on
a bad address.

David

Re: [PATCH 2.6.30-rc5] ehea: fix invalid pointer access

[Hannes Hering]

On Tuesday 05 May 2009 14:19:54 David Howells wrote:
> In that case, you might want to move the prefetchw() calls in the following:
> 
> 		pref = skb_array[x];
> 	-	prefetchw(pref);
> 	-	prefetchw(pref + EHEA_CACHE_LINE);
> 	+	if (pref) {
> 	+		prefetchw(pref);
> 	+		prefetchw(pref + EHEA_CACHE_LINE);
> 
> to before the if-statement.  That way the CPU can be attempting the prefetch
> whilst it's chewing over the test and branch.  prefetching shouldn't fault on
> a bad address.
> 
> David
Hi David,

you are right so far, but actually the prefetch calls on POWER also contain
an if statement to check if the address is valid (i. e. non-zero). We never
have the case that the pref != NULL and pref->data == NULL. And the situation
of pref==NULL is very rare. This means there is no benefit moving our if
statement down from performance perspective if we assume that our if does not
take longer then the if in the prefetch command. We can add an
if(likely(pref) if you like. In fact doing the if statement as we do it now
we actually save the prefetch if statements in case we hit the situation of
pref==NULL.

Regards

Hannes