[PATCH] sbp2: fix check of return value of hpsb_allocate_and_register_addrspace

[PATCH] sbp2: fix check of return value of hpsb_allocate_and_register_addrspace

[Stefan Richter]

I added a failure check in patch "sbp2: variable status FIFO address
(fix login timeout)" --- alas for a wrong error value.  This is a bug
since Linux 2.6.16.  Leads to NULL pointer dereference if the call
failed, and bogus failure handling if call succeeded.

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
applies to 2.6.17-rc5
applies to 2.6.16.x after patch ''ohci1394, sbp2: fix "scsi_add_device
failed" with PL-3507 based devices''

Index: linux-2.6.17-rc5/drivers/ieee1394/sbp2.c
===================================================================
--- linux-2.6.17-rc5.orig/drivers/ieee1394/sbp2.c	2006-06-03 01:52:54.000000000 +0200
+++ linux-2.6.17-rc5/drivers/ieee1394/sbp2.c	2006-06-03 01:54:23.000000000 +0200
@@ -845,7 +845,7 @@ static struct scsi_id_instance_data *sbp
 			&sbp2_highlevel, ud->ne->host, &sbp2_ops,
 			sizeof(struct sbp2_status_block), sizeof(quadlet_t),
 			0x010000000000ULL, CSR1212_ALL_SPACE_END);
-	if (!scsi_id->status_fifo_addr) {
+	if (scsi_id->status_fifo_addr == ~0ULL) {
 		SBP2_ERR("failed to allocate status FIFO address range");
 		goto failed_alloc;
 	}

Re: [stable] [PATCH] sbp2: fix check of return value of hpsb_allocate_and_register_addrspace

[Chris Wright]

* Stefan Richter (stefanr@s5r6.in-berlin.de) wrote:
> I added a failure check in patch "sbp2: variable status FIFO address
> (fix login timeout)" --- alas for a wrong error value.  This is a bug
> since Linux 2.6.16.  Leads to NULL pointer dereference if the call
> failed, and bogus failure handling if call succeeded.
> 
> Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
> ---
> applies to 2.6.17-rc5
> applies to 2.6.16.x after patch ''ohci1394, sbp2: fix "scsi_add_device
> failed" with PL-3507 based devices''
> 
> Index: linux-2.6.17-rc5/drivers/ieee1394/sbp2.c
> ===================================================================
> --- linux-2.6.17-rc5.orig/drivers/ieee1394/sbp2.c	2006-06-03 01:52:54.000000000 +0200
> +++ linux-2.6.17-rc5/drivers/ieee1394/sbp2.c	2006-06-03 01:54:23.000000000 +0200
> @@ -845,7 +845,7 @@ static struct scsi_id_instance_data *sbp
>  			&sbp2_highlevel, ud->ne->host, &sbp2_ops,
>  			sizeof(struct sbp2_status_block), sizeof(quadlet_t),
>  			0x010000000000ULL, CSR1212_ALL_SPACE_END);
> -	if (!scsi_id->status_fifo_addr) {
> +	if (scsi_id->status_fifo_addr == ~0ULL) {
>  		SBP2_ERR("failed to allocate status FIFO address range");
>  		goto failed_alloc;
>  	}
> 

Is that enough?

failed_alloc:
        sbp2_remove_device(scsi_id);

sbp2_remove_device(scsi_id)
  if (scsi_id->status_fifo_addr)
    hpsb_unregister_addrspace()

Suppose status_fifo_addr won't match any as->start.

Re: [stable] [PATCH] sbp2: fix check of return value of hpsb_allocate_and_register_addrspace

[Stefan Richter]

Chris Wright wrote:
> * Stefan Richter (stefanr@s5r6.in-berlin.de) wrote:
...
>>+++ linux-2.6.17-rc5/drivers/ieee1394/sbp2.c	2006-06-03 01:54:23.000000000 +0200
>>@@ -845,7 +845,7 @@ static struct scsi_id_instance_data *sbp
>> 			&sbp2_highlevel, ud->ne->host, &sbp2_ops,
>> 			sizeof(struct sbp2_status_block), sizeof(quadlet_t),
>> 			0x010000000000ULL, CSR1212_ALL_SPACE_END);
>>-	if (!scsi_id->status_fifo_addr) {
>>+	if (scsi_id->status_fifo_addr == ~0ULL) {
>> 		SBP2_ERR("failed to allocate status FIFO address range");
>> 		goto failed_alloc;
>> 	}
>>
> 
> 
> Is that enough?
> 
> failed_alloc:
>         sbp2_remove_device(scsi_id);
> 
> sbp2_remove_device(scsi_id)
>   if (scsi_id->status_fifo_addr)
>     hpsb_unregister_addrspace()
> 
> Suppose status_fifo_addr won't match any as->start.

Thanks, here is another bug. An address space beginning at 0 won't be 
de-registered. But this is not a big issue because 1. a configuration 
where a FIFO address space starting from 0 is impractical anyway (can 
occur if CONFIG_IEEE1394_SBP2_PHYS_DMA=N and physical DMA is unavailable 
from the host adapter, which won't work at the moment) and 2. the 
address space is a plenty resource (both as a bus address and with 
respect to the backing data structures) and 3. would be unregistered if 
the sbp2 module was unloaded. This is not critical for -stable.

On the other hand, if hpsb_unregister_addrspace(HL_driver, host, 
address) with address == ~0ULL (i.e. 
hpsb_allocate_and_register_addrspace failed before), it would do nothing 
but burn a few CPU cycles unsuccessfully searching for an address space 
starting at ~0ULL. Valid address spaces start at an address lower than 
CSR1212_ALL_SPACE_END == 1ULL << 48.

I will post a follow-up patch after breakfast, but it isn't relevant for 
-stable.

Thanks,
-- 
Stefan Richter
-=====-=-==- -==- ---==
http://arcgraph.de/sr/

Re: [stable] [PATCH] sbp2: fix check of return value of hpsb_allocate_and_register_addrspace

[Michael Tokarev]

Stefan Richter wrote:
> Chris Wright wrote:
>> * Stefan Richter (stefanr@s5r6.in-berlin.de) wrote:
> ....
>>> +++ linux-2.6.17-rc5/drivers/ieee1394/sbp2.c    2006-06-03
>>> 01:54:23.000000000 +0200
>>> @@ -845,7 +845,7 @@ static struct scsi_id_instance_data *sbp
>>>             &sbp2_highlevel, ud->ne->host, &sbp2_ops,
>>>             sizeof(struct sbp2_status_block), sizeof(quadlet_t),
>>>             0x010000000000ULL, CSR1212_ALL_SPACE_END);
>>> -    if (!scsi_id->status_fifo_addr) {
>>> +    if (scsi_id->status_fifo_addr == ~0ULL) {

Umm.  Can this ~0ULL constant be #define'd to something?
It's way too simple to mis-read it as NULL (or ~NULL whatever).

I mean.. I looked at this change for quite some time, trying to
figure a difference (!x vs x==NULL), and thinking what's '-0'
(~ turned out to be quite similar to - in the font my thunderbird
uses).

/mjt

Re: [stable] [PATCH] sbp2: fix check of return value of hpsb_allocate_and_register_addrspace

[Andreas Schwab]

Michael Tokarev <mjt@tls.msk.ru> writes:

> Stefan Richter wrote:
>> Chris Wright wrote:
>>> * Stefan Richter (stefanr@s5r6.in-berlin.de) wrote:
>> ....
>>>> +++ linux-2.6.17-rc5/drivers/ieee1394/sbp2.c    2006-06-03
>>>> 01:54:23.000000000 +0200
>>>> @@ -845,7 +845,7 @@ static struct scsi_id_instance_data *sbp
>>>>             &sbp2_highlevel, ud->ne->host, &sbp2_ops,
>>>>             sizeof(struct sbp2_status_block), sizeof(quadlet_t),
>>>>             0x010000000000ULL, CSR1212_ALL_SPACE_END);
>>>> -    if (!scsi_id->status_fifo_addr) {
>>>> +    if (scsi_id->status_fifo_addr == ~0ULL) {
>
> Umm.  Can this ~0ULL constant be #define'd to something?
> It's way too simple to mis-read it as NULL (or ~NULL whatever).

How about writing it as -1?

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [stable] [PATCH] sbp2: fix check of return value of hpsb_allocate_and_register_addrspace

[Andrew Morton]

On Sat, 03 Jun 2006 11:31:27 +0200
Andreas Schwab <schwab@suse.de> wrote:

> Michael Tokarev <mjt@tls.msk.ru> writes:
> 
> > Stefan Richter wrote:
> >> Chris Wright wrote:
> >>> * Stefan Richter (stefanr@s5r6.in-berlin.de) wrote:
> >> ....
> >>>> +++ linux-2.6.17-rc5/drivers/ieee1394/sbp2.c    2006-06-03
> >>>> 01:54:23.000000000 +0200
> >>>> @@ -845,7 +845,7 @@ static struct scsi_id_instance_data *sbp
> >>>>             &sbp2_highlevel, ud->ne->host, &sbp2_ops,
> >>>>             sizeof(struct sbp2_status_block), sizeof(quadlet_t),
> >>>>             0x010000000000ULL, CSR1212_ALL_SPACE_END);
> >>>> -    if (!scsi_id->status_fifo_addr) {
> >>>> +    if (scsi_id->status_fifo_addr == ~0ULL) {
> >
> > Umm.  Can this ~0ULL constant be #define'd to something?
> > It's way too simple to mis-read it as NULL (or ~NULL whatever).
> 
> How about writing it as -1?
> 

That's preferable.

It doesn't actually cause a problem, but status_fifo_addr is defined as
u64, which is not `unsigned long long'.  On powerpc, for example, u64 is
implemented as unsigned long.  -1 just works.

Re: [stable] [PATCH] sbp2: fix check of return value of hpsb_allocate_and_register_addrspace

[Stefan Richter]

Andrew Morton wrote:
> On Sat, 03 Jun 2006 11:31:27 +0200
> Andreas Schwab <schwab@suse.de> wrote:
>>Michael Tokarev <mjt@tls.msk.ru> writes:
>>>>>* Stefan Richter (stefanr@s5r6.in-berlin.de) wrote:
>>>>>>-    if (!scsi_id->status_fifo_addr) {
>>>>>>+    if (scsi_id->status_fifo_addr == ~0ULL) {
>>>
>>>Umm.  Can this ~0ULL constant be #define'd to something?
>>>It's way too simple to mis-read it as NULL (or ~NULL whatever).
>>
>>How about writing it as -1?
> 
> That's preferable.
> 
> It doesn't actually cause a problem, but status_fifo_addr is defined as
> u64, which is not `unsigned long long'.  On powerpc, for example, u64 is
> implemented as unsigned long.  -1 just works.

I have a patch ready which replaces the magic value by a sensibly named 
preprocessor constant. Will be posted within the hour. Thanks for the 
comments.
-- 
Stefan Richter
-=====-=-==- -==- ---==
http://arcgraph.de/sr/

[PATCH 1/2] sbp2: fix deregistration of status fifo address space

[Stefan Richter]

The proper designator of an invalid CSR address is ~(u64)0, not (u64)0.
Use the correct value in initialization and deregistration.
Also, scsi_id->sbp2_lun does not need to be initialized twice.
(scsi_id was kzalloc'd.)

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>

Index: linux/drivers/ieee1394/sbp2.c
===================================================================
--- linux.orig/drivers/ieee1394/sbp2.c	2006-06-03 02:13:18.000000000 +0200
+++ linux/drivers/ieee1394/sbp2.c	2006-06-03 11:49:18.000000000 +0200
@@ -794,12 +794,12 @@ static struct scsi_id_instance_data *sbp
 	scsi_id->ud = ud;
 	scsi_id->speed_code = IEEE1394_SPEED_100;
 	scsi_id->max_payload_size = sbp2_speedto_max_payload[IEEE1394_SPEED_100];
+	scsi_id->status_fifo_addr = ~0ULL;
 	atomic_set(&scsi_id->sbp2_login_complete, 0);
 	INIT_LIST_HEAD(&scsi_id->sbp2_command_orb_inuse);
 	INIT_LIST_HEAD(&scsi_id->sbp2_command_orb_completed);
 	INIT_LIST_HEAD(&scsi_id->scsi_list);
 	spin_lock_init(&scsi_id->sbp2_command_orb_lock);
-	scsi_id->sbp2_lun = 0;
 
 	ud->device.driver_data = scsi_id;
 
@@ -1090,7 +1090,7 @@ static void sbp2_remove_device(struct sc
 		SBP2_DMA_FREE("single query logins data");
 	}
 
-	if (scsi_id->status_fifo_addr)
+	if (scsi_id->status_fifo_addr != ~0ULL)
 		hpsb_unregister_addrspace(&sbp2_highlevel, hi->host,
 			scsi_id->status_fifo_addr);
 

[PATCH 2/2] ieee1394: add preprocessor constant for invalid csr address

[Stefan Richter]

Replace occurrences of the magic value ~(u64)0 for invalid
CSR address spaces by a named constant for better readability.

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
 drivers/ieee1394/csr1212.c   |    2 +-
 drivers/ieee1394/csr1212.h   |    1 +
 drivers/ieee1394/highlevel.c |    9 +++++----
 drivers/ieee1394/eth1394.c   |   16 +++++++---------
 drivers/ieee1394/eth1394.h   |    2 --
 drivers/ieee1394/sbp2.c      |    8 ++++----
 6 files changed, 18 insertions(+), 20 deletions(-)

Index: linux/drivers/ieee1394/csr1212.h
===================================================================
--- linux.orig/drivers/ieee1394/csr1212.h	2006-04-24 22:20:24.000000000 +0200
+++ linux/drivers/ieee1394/csr1212.h	2006-06-03 12:52:32.000000000 +0200
@@ -192,6 +192,7 @@
 
 #define  CSR1212_EXTENDED_ROM_SIZE		(0x10000 * sizeof(u_int32_t))
 
+#define  CSR1212_INVALID_ADDR_SPACE		-1
 
 /* Config ROM image structures */
 struct csr1212_bus_info_block_img {
Index: linux/drivers/ieee1394/csr1212.c
===================================================================
--- linux.orig/drivers/ieee1394/csr1212.c	2006-04-24 22:20:24.000000000 +0200
+++ linux/drivers/ieee1394/csr1212.c	2006-06-03 12:12:20.000000000 +0200
@@ -779,7 +779,7 @@ static int csr1212_append_new_cache(stru
 	romsize = (romsize + (csr->max_rom - 1)) & ~(csr->max_rom - 1);
 
 	csr_addr = csr->ops->allocate_addr_range(romsize, csr->max_rom, csr->private);
-	if (csr_addr == ~0ULL) {
+	if (csr_addr == CSR1212_INVALID_ADDR_SPACE) {
 		return CSR1212_ENOMEM;
 	}
 	if (csr_addr < CSR1212_REGISTER_SPACE_BASE) {
Index: linux/drivers/ieee1394/highlevel.c
===================================================================
--- linux.orig/drivers/ieee1394/highlevel.c	2006-06-03 02:13:18.000000000 +0200
+++ linux/drivers/ieee1394/highlevel.c	2006-06-03 12:18:23.000000000 +0200
@@ -301,7 +301,7 @@ u64 hpsb_allocate_and_register_addrspace
 {
 	struct hpsb_address_serve *as, *a1, *a2;
 	struct list_head *entry;
-	u64 retval = ~0ULL;
+	u64 retval = CSR1212_INVALID_ADDR_SPACE;
 	unsigned long flags;
 	u64 align_mask = ~(alignment - 1);
 
@@ -315,9 +315,10 @@ u64 hpsb_allocate_and_register_addrspace
 
 	/* default range,
 	 * avoids controller's posted write area (see OHCI 1.1 clause 1.5) */
-	if (start == ~0ULL && end == ~0ULL) {
+	if (start == CSR1212_INVALID_ADDR_SPACE &&
+	    end   == CSR1212_INVALID_ADDR_SPACE) {
 		start = host->middle_addr_space;
-		end = CSR1212_ALL_SPACE_END;
+		end   = CSR1212_ALL_SPACE_END;
 	}
 
 	if (((start|end) & ~align_mask) || (start >= end) || (end > 0x1000000000000ULL)) {
@@ -361,7 +362,7 @@ u64 hpsb_allocate_and_register_addrspace
 
 	write_unlock_irqrestore(&addr_space_lock, flags);
 
-	if (retval == ~0ULL) {
+	if (retval == CSR1212_INVALID_ADDR_SPACE) {
 		kfree(as);
 	}
 
Index: linux/drivers/ieee1394/eth1394.h
===================================================================
--- linux.orig/drivers/ieee1394/eth1394.h	2006-04-24 22:20:24.000000000 +0200
+++ linux/drivers/ieee1394/eth1394.h	2006-06-03 12:14:40.000000000 +0200
@@ -32,8 +32,6 @@
  * S3200 (per Table 16-3 of IEEE 1394b-2002). */
 #define ETHER1394_REGION_ADDR_LEN	4096
 
-#define ETHER1394_INVALID_ADDR		~0ULL
-
 /* GASP identifier numbers for IPv4 over IEEE 1394 */
 #define ETHER1394_GASP_SPECIFIER_ID	0x00005E
 #define ETHER1394_GASP_SPECIFIER_ID_HI	((ETHER1394_GASP_SPECIFIER_ID >> 8) & 0xffff)
Index: linux/drivers/ieee1394/eth1394.c
===================================================================
--- linux.orig/drivers/ieee1394/eth1394.c	2006-06-03 02:13:18.000000000 +0200
+++ linux/drivers/ieee1394/eth1394.c	2006-06-03 12:22:44.000000000 +0200
@@ -367,7 +367,7 @@ static int eth1394_probe(struct device *
 	spin_lock_init(&node_info->pdg.lock);
 	INIT_LIST_HEAD(&node_info->pdg.list);
 	node_info->pdg.sz = 0;
-	node_info->fifo = ETHER1394_INVALID_ADDR;
+	node_info->fifo = CSR1212_INVALID_ADDR_SPACE;
 
 	ud->device.driver_data = node_info;
 	new_node->ud = ud;
@@ -566,13 +566,11 @@ static void ether1394_add_host (struct h
 	if (!(host->config_roms & HPSB_CONFIG_ROM_ENTRY_IP1394))
 		return;
 
-	fifo_addr = hpsb_allocate_and_register_addrspace(&eth1394_highlevel,
-							 host,
-							 &addr_ops,
-							 ETHER1394_REGION_ADDR_LEN,
-							 ETHER1394_REGION_ADDR_LEN,
-							 -1, -1);
-	if (fifo_addr == ~0ULL)
+	fifo_addr = hpsb_allocate_and_register_addrspace(
+			&eth1394_highlevel, host, &addr_ops,
+			ETHER1394_REGION_ADDR_LEN, ETHER1394_REGION_ADDR_LEN,
+			CSR1212_INVALID_ADDR_SPACE, CSR1212_INVALID_ADDR_SPACE);
+	if (fifo_addr == CSR1212_INVALID_ADDR_SPACE)
 		goto out;
 
 	/* We should really have our own alloc_hpsbdev() function in
@@ -1686,7 +1684,7 @@ static int ether1394_tx (struct sk_buff 
 			goto fail;
 		}
 		node_info = (struct eth1394_node_info*)node->ud->device.driver_data;
-		if (node_info->fifo == ETHER1394_INVALID_ADDR) {
+		if (node_info->fifo == CSR1212_INVALID_ADDR_SPACE) {
 			ret = -EAGAIN;
 			goto fail;
 		}
Index: linux/drivers/ieee1394/sbp2.c
===================================================================
--- linux.orig/drivers/ieee1394/sbp2.c	2006-06-03 11:49:18.000000000 +0200
+++ linux/drivers/ieee1394/sbp2.c	2006-06-03 12:27:16.000000000 +0200
@@ -794,7 +794,7 @@ static struct scsi_id_instance_data *sbp
 	scsi_id->ud = ud;
 	scsi_id->speed_code = IEEE1394_SPEED_100;
 	scsi_id->max_payload_size = sbp2_speedto_max_payload[IEEE1394_SPEED_100];
-	scsi_id->status_fifo_addr = ~0ULL;
+	scsi_id->status_fifo_addr = CSR1212_INVALID_ADDR_SPACE;
 	atomic_set(&scsi_id->sbp2_login_complete, 0);
 	INIT_LIST_HEAD(&scsi_id->sbp2_command_orb_inuse);
 	INIT_LIST_HEAD(&scsi_id->sbp2_command_orb_completed);
@@ -848,7 +848,7 @@ static struct scsi_id_instance_data *sbp
 			&sbp2_highlevel, ud->ne->host, &sbp2_ops,
 			sizeof(struct sbp2_status_block), sizeof(quadlet_t),
 			ud->ne->host->low_addr_space, CSR1212_ALL_SPACE_END);
-	if (scsi_id->status_fifo_addr == ~0ULL) {
+	if (scsi_id->status_fifo_addr == CSR1212_INVALID_ADDR_SPACE) {
 		SBP2_ERR("failed to allocate status FIFO address range");
 		goto failed_alloc;
 	}
@@ -1090,9 +1090,9 @@ static void sbp2_remove_device(struct sc
 		SBP2_DMA_FREE("single query logins data");
 	}
 
-	if (scsi_id->status_fifo_addr != ~0ULL)
+	if (scsi_id->status_fifo_addr != CSR1212_INVALID_ADDR_SPACE)
 		hpsb_unregister_addrspace(&sbp2_highlevel, hi->host,
-			scsi_id->status_fifo_addr);
+					  scsi_id->status_fifo_addr);
 
 	scsi_id->ud->device.driver_data = NULL;