zfcp: Test kmalloc failure in scsi_get_vpd_page()
PATCH 1/1 zfcp: Test kmalloc failure in scsi_get_vpd_page()
kmalloc() may fail, so test whether it succeeded.
---
drivers/scsi/scsi.c +3 -0
@@ -1056,6 +1056,9 @@ unsigned char *scsi_get_vpd_page(struct scsi_device *sdev, u8 page)kfree (buf); buf =kmalloc (len +4 ,GFP_KERNEL ); +if (!buf) +return NULL ; + result =scsi_vpd_inquiry (sdev, buf, page, len);if (result) goto fail;
Quoted 9 lines · Roel Kluin
kmalloc()may fail, so test whether it succeeded.Signed-off-by Roel Kluin <[email protected]>---
drivers/scsi/scsi.c +0
-0
drivers/scsi/scsi.c unsigned char *scsi_get_vpd_page(struct scsi_device *sdev, u8 page) · line 1061
Quoted 6 lines
drivers/scsi/scsi.c +3
-0@@ -1056,6 +1056,9 @@ unsigned char *scsi_get_vpd_page(struct scsi_device *sdev, u8 page)kfree (buf); buf =kmalloc (len +4 ,GFP_KERNEL ); +if (!buf) +return NULL ; +
Firstly, this won't actually apply ... you should be developing against either the SCSI trees or linux-next to get the latest versions in git.
Secondly it's not really right for the most common use cases, which don't usually want the whole vpd buffer anyway. I don't really see a simple way of fixing it without altering the interface, though.
James
---
Quoted 20 lines · James Bottomley
Quoted 9 lines · Roel Kluin
kmalloc()may fail, so test whether it succeeded.Signed-off-by Roel Kluin <[email protected]>---
drivers/scsi/scsi.c +0
-0drivers/scsi/scsi.c unsigned char *scsi_get_vpd_page(struct scsi_device *sdev, u8 page) · line 1061
Quoted 6 lines
drivers/scsi/scsi.c +3
-0@@ -1056,6 +1056,9 @@ unsigned char *scsi_get_vpd_page(struct scsi_device *sdev, u8 page)kfree (buf); buf =kmalloc (len +4 ,GFP_KERNEL ); +if (!buf) +return NULL ; +Firstly, this won't actually apply ... you should be developing against either the SCSI trees or linux-next to get the latest versions in git.
Quoted 4 lines
Secondly it's not really right for the most common use cases, which don't usually want the whole vpd buffer anyway. I don't really see a simple way of fixing it without altering the interface, though.
"most common use cases" do not have pages bigger then 255. Can you think of any? For these few places that anticipate pages bigger then 255 I don't see much choice, we just freed 255 bytes and tried a new size, say 317, which failed with GFP_KERNEL, In such a busy system, better fail with NULL then BUG, No? In any way caller must check for NULL because of the first allocation.
Quoted 2 lines
James
Boaz
Quoted 26 lines · Boaz Harrosh
Quoted 20 lines · James Bottomley
Quoted 9 lines · Roel Kluin
kmalloc()may fail, so test whether it succeeded.Signed-off-by Roel Kluin <[email protected]>---
drivers/scsi/scsi.c +0
-0drivers/scsi/scsi.c unsigned char *scsi_get_vpd_page(struct scsi_device *sdev, u8 page) · line 1061
Quoted 6 lines
drivers/scsi/scsi.c +3
-0@@ -1056,6 +1056,9 @@ unsigned char *scsi_get_vpd_page(struct scsi_device *sdev, u8 page)kfree (buf); buf =kmalloc (len +4 ,GFP_KERNEL ); +if (!buf) +return NULL ; +Firstly, this won't actually apply ... you should be developing against either the SCSI trees or linux-next to get the latest versions in git.
Quoted 4 lines
Secondly it's not really right for the most common use cases, which don't usually want the whole vpd buffer anyway. I don't really see a simple way of fixing it without altering the interface, though.
Quoted 5 lines
"most common use cases" do not have pages bigger then 255. Can you think of any? For these few places that anticipate pages bigger then 255 I don't see much choice, we just freed 255 bytes and tried a new size, say 317, which failed with
GFP_KERNEL, In such a busy system, better fail with NULL then BUG, No? In any way caller must check for NULL because of the first allocation.
Other way around: common use cases means the callers. What I'm saying is that regardless of the size of the VPD page, the caller usually only wants a few bytes into it. Once we have all the information the caller ever needs, there's no point in trying again with a larger buffer just because the VPD page is larger ... to code this into the interface, though, the caller would need to specify the max length it is looking for.
James
Quoted 33 lines · James Bottomley
Quoted 26 lines · Boaz Harrosh
Quoted 20 lines · James Bottomley
Quoted 9 lines · Roel Kluin
kmalloc()may fail, so test whether it succeeded.Signed-off-by Roel Kluin <[email protected]>---
drivers/scsi/scsi.c +0
-0drivers/scsi/scsi.c unsigned char *scsi_get_vpd_page(struct scsi_device *sdev, u8 page) · line 1061
Quoted 6 lines
drivers/scsi/scsi.c +3
-0@@ -1056,6 +1056,9 @@ unsigned char *scsi_get_vpd_page(struct scsi_device *sdev, u8 page)kfree (buf); buf =kmalloc (len +4 ,GFP_KERNEL ); +if (!buf) +return NULL ; +Firstly, this won't actually apply ... you should be developing against either the SCSI trees or linux-next to get the latest versions in git.
Quoted 4 lines
Secondly it's not really right for the most common use cases, which don't usually want the whole vpd buffer anyway. I don't really see a simple way of fixing it without altering the interface, though.
Quoted 5 lines
"most common use cases" do not have pages bigger then 255. Can you think of any? For these few places that anticipate pages bigger then 255 I don't see much choice, we just freed 255 bytes and tried a new size, say 317, which failed with
GFP_KERNEL, In such a busy system, better fail with NULL then BUG, No? In any way caller must check for NULL because of the first allocation.
Quoted 7 lines
Other way around: common use cases means the callers. What I'm saying is that regardless of the size of the VPD page, the caller usually only wants a few bytes into it. Once we have all the information the caller ever needs, there's no point in trying again with a larger buffer just because the VPD page is larger ... to code this into the interface, though, the caller would need to specify the max length it is looking for.
OK, it's been a couple of months and no patch has been forthcoming on this, so how about the attached? It does the query and only adjusts the size where the caller actually wants to bother.
James
---
drivers/scsi/scsi.c +12 -28
@@ -1026,55 +1026,39 @@ static int scsi_vpd_inquiry(struct scsi_device *sdev, unsigned char *buffer,* responsible for calling kfree() on this pointer when it is no longer * needed. If we cannot retrieve the VPD page this routine returns %NULL. */ -unsigned char *scsi_get_vpd_page (struct scsi_device *sdev,u8 page) +int scsi_get_vpd_page (struct scsi_device *sdev,u8 page,unsigned char *buf, +int buf_len) {int i, result; -unsigned int len; -const unsigned int init_vpd_len =255 ; -unsigned char *buf =kmalloc (init_vpd_len,GFP_KERNEL ); - -if (!buf) -return NULL ;/* Ask for all the pages supported by this device */ - result =scsi_vpd_inquiry (sdev, buf,0 , init_vpd_len); + result =scsi_vpd_inquiry (sdev, buf,0 , buf_len);if (result) goto fail;/* If the user actually wanted this page, we can skip the rest */ if (page ==0 ) -return buf; +return -EINVAL ; -for (i =0 ; i < buf[3 ]; i++) +for (i =0 ; i <min ((int )buf[3 ], buf_len -4 ); i++)if (buf[i +4 ] == page) goto found; + +if (i < buf[3 ] && i > buf_len) +/* ran off the end of the buffer, give us benefit of doubt */ + goto found;/* The device claims it doesn't support the requested page */ goto fail; found: - result =scsi_vpd_inquiry (sdev, buf, page,255 ); + result =scsi_vpd_inquiry (sdev, buf, page, buf_len);if (result) goto fail; -/* -* Some pages are longer than 255 bytes. The actual length of -* the page is returned in the header. -*/ - len = ((buf[2 ] <<8 ) | buf[3 ]) +4 ; -if (len <= init_vpd_len) -return buf; - -kfree (buf); - buf =kmalloc (len,GFP_KERNEL ); - result =scsi_vpd_inquiry (sdev, buf, page, len); -if (result) - goto fail; - -return buf; +return 0 ; fail: -kfree (buf); -return NULL ; +return -EINVAL ; }EXPORT_SYMBOL_GPL (scsi_get_vpd_page);
drivers/scsi/sd.c +15 -11
@@ -1864,19 +1864,20 @@ void sd_read_app_tag_own(struct scsi_disk *sdkp, unsigned char *buffer)static void sd_read_block_limits (struct scsi_disk *sdkp) {unsigned int sector_sz = sdkp->device->sector_size; -char *buffer; +const int vpd_len =32 ; +unsigned char *buffer =kmalloc (vpd_len,GFP_KERNEL ); -/* Block Limits VPD */ - buffer =scsi_get_vpd_page (sdkp->device,0xb0 ); - -if (buffer ==NULL ) -return ; +if (!buffer || +/* Block Limits VPD */ +scsi_get_vpd_page (sdkp->device,0xb0 , buffer, vpd_len)) + goto out;blk_queue_io_min (sdkp->disk->queue,get_unaligned_be16 (&buffer[6 ]) * sector_sz);blk_queue_io_opt (sdkp->disk->queue,get_unaligned_be32 (&buffer[12 ]) * sector_sz); + out:kfree (buffer); }
@@ -1886,20 +1887,23 @@ static void sd_read_block_limits(struct scsi_disk *sdkp)*/ static void sd_read_block_characteristics (struct scsi_disk *sdkp) { -char *buffer; +unsigned char *buffer;u16 rot; +const int vpd_len =32 ; -/* Block Device Characteristics VPD */ - buffer =scsi_get_vpd_page (sdkp->device,0xb1 ); + buffer =kmalloc (vpd_len,GFP_KERNEL ); -if (buffer ==NULL ) -return ; +if (!buffer || +/* Block Device Characteristics VPD */ +scsi_get_vpd_page (sdkp->device,0xb1 , buffer, vpd_len)) + goto out; rot =get_unaligned_be16 (&buffer[4 ]);if (rot ==1 )queue_flag_set_unlocked (QUEUE_FLAG_NONROT , sdkp->disk->queue); + out:kfree (buffer); }
drivers/scsi/ses.c +7 -3
@@ -448,13 +448,17 @@ static void ses_match_to_enclosure(struct enclosure_device *edev, .addr = 0, }; - buf =scsi_get_vpd_page (sdev,0x83 ); -if (!buf) -return ; + buf =kmalloc (INIT_ALLOC_SIZE ,GFP_KERNEL ); +if (!buf ||scsi_get_vpd_page (sdev,0x83 , buf,INIT_ALLOC_SIZE )) + goto free;ses_enclosure_data_process (edev,to_scsi_device (edev->edev.parent),0 ); vpd_len = ((buf[2 ] <<8 ) | buf[3 ]) +4 ; +kfree (buf); + buf =kmalloc (vpd_len,GFP_KERNEL ); +if (!buf ||scsi_get_vpd_page (sdev,0x83 , buf, vpd_len)) + goto free; desc = buf +4 ;while (desc < buf + vpd_len) {
include/scsi/scsi_device.h +2 -1
@@ -347,7 +347,8 @@ extern int scsi_mode_select(struct scsi_device *sdev, int pf, int sp, struct scsi_sense_hdr *);extern int scsi_test_unit_ready (struct scsi_device *sdev,int timeout,int retries,struct scsi_sense_hdr *sshdr); -extern unsigned char *scsi_get_vpd_page (struct scsi_device *,u8 page); +extern int scsi_get_vpd_page (struct scsi_device *,u8 page,unsigned char *buf, +int buf_len);extern int scsi_device_set_state (struct scsi_device *sdev,enum scsi_device_state state); extern struct scsi_event *sdev_evt_alloc(enum scsi_device_event evt_type,
Quoted 55 lines · James Bottomley
Quoted 33 lines · James Bottomley
Quoted 26 lines · Boaz Harrosh
Quoted 20 lines · James Bottomley
Quoted 9 lines · Roel Kluin
kmalloc()may fail, so test whether it succeeded.Signed-off-by Roel Kluin <[email protected]>---
drivers/scsi/scsi.c +0
-0drivers/scsi/scsi.c unsigned char *scsi_get_vpd_page(struct scsi_device *sdev, u8 page) · line 1061
Quoted 6 lines
drivers/scsi/scsi.c +3
-0@@ -1056,6 +1056,9 @@ unsigned char *scsi_get_vpd_page(struct scsi_device *sdev, u8 page)kfree (buf); buf =kmalloc (len +4 ,GFP_KERNEL ); +if (!buf) +return NULL ; +Firstly, this won't actually apply ... you should be developing against either the SCSI trees or linux-next to get the latest versions in git.
Quoted 4 lines
Secondly it's not really right for the most common use cases, which don't usually want the whole vpd buffer anyway. I don't really see a simple way of fixing it without altering the interface, though.
Quoted 5 lines
"most common use cases" do not have pages bigger then 255. Can you think of any? For these few places that anticipate pages bigger then 255 I don't see much choice, we just freed 255 bytes and tried a new size, say 317, which failed with
GFP_KERNEL, In such a busy system, better fail with NULL then BUG, No? In any way caller must check for NULL because of the first allocation.Quoted 7 lines
Other way around: common use cases means the callers. What I'm saying is that regardless of the size of the VPD page, the caller usually only wants a few bytes into it. Once we have all the information the caller ever needs, there's no point in trying again with a larger buffer just because the VPD page is larger ... to code this into the interface, though, the caller would need to specify the max length it is looking for.
OK, it's been a couple of months and no patch has been forthcoming on this, so how about the attached? It does the query and only adjusts the size where the caller actually wants to bother.
James
---
drivers/scsi/scsi.c +0
-0
drivers/scsi/scsi.c static int scsi_vpd_inquiry(struct scsi_device *sdev, unsigned char *buffer, · line 1028
Quoted 3 lines
drivers/scsi/scsi.c +0
-0@@ -1026,55 +1026,39 @@ static int scsi_vpd_inquiry(struct scsi_device *sdev, unsigned char *buffer,* responsible for calling kfree() on this pointer when it is no longer * needed. If we cannot retrieve the VPD page this routine returns %NULL. */
The comment is wrong now, I would also say something about buf_len is recommended to be 255 or more since other wise we might miss out on pages in the first inquiry.
Quoted 13 lines
drivers/scsi/scsi.c +2
-7-unsigned char *scsi_get_vpd_page (struct scsi_device *sdev,u8 page) +int scsi_get_vpd_page (struct scsi_device *sdev,u8 page,unsigned char *buf, +int buf_len) {int i, result; -unsigned int len; -const unsigned int init_vpd_len =255 ; -unsigned char *buf =kmalloc (init_vpd_len,GFP_KERNEL ); - -if (!buf) -return NULL ;/* Ask for all the pages supported by this device */
drivers/scsi/scsi.c static int scsi_vpd_inquiry(struct scsi_device *sdev, unsigned char *buffer, · line 1042
Quoted 10 lines
drivers/scsi/scsi.c +2
-2- result =scsi_vpd_inquiry (sdev, buf,0 , init_vpd_len); + result =scsi_vpd_inquiry (sdev, buf,0 , buf_len);if (result) goto fail;/* If the user actually wanted this page, we can skip the rest */ if (page ==0 ) -return buf; +return -EINVAL ;
Why -EINVAL; Look at the comment above, return 0 would be better.
drivers/scsi/scsi.c static int scsi_vpd_inquiry(struct scsi_device *sdev, unsigned char *buffer, · line 1049
Quoted 8 lines
drivers/scsi/scsi.c +5
-1-for (i =0 ; i < buf[3 ]; i++) +for (i =0 ; i <min ((int )buf[3 ], buf_len -4 ); i++)if (buf[i +4 ] == page) goto found; + +if (i < buf[3 ] && i > buf_len) +/* ran off the end of the buffer, give us benefit of doubt */ + goto found;
Some cheep devices are known to break when asked for pages they do not support better return an -ETOOSMALL the user can check for. (And the comment above should also take care of it)
Quoted 20 lines
drivers/scsi/scsi.c +1
-12/* The device claims it doesn't support the requested page */ goto fail; found: - result =scsi_vpd_inquiry (sdev, buf, page,255 ); + result =scsi_vpd_inquiry (sdev, buf, page, buf_len);if (result) goto fail; -/* -* Some pages are longer than 255 bytes. The actual length of -* the page is returned in the header. -*/ - len = ((buf[2 ] <<8 ) | buf[3 ]) +4 ; -if (len <= init_vpd_len) -return buf; - -kfree (buf); - buf =kmalloc (len,GFP_KERNEL ); - result =scsi_vpd_inquiry (sdev, buf, page, len);
drivers/scsi/scsi.c static int scsi_vpd_inquiry(struct scsi_device *sdev, unsigned char *buffer, · line 1061
Quoted 10 lines
drivers/scsi/scsi.c +2
-6-if (result) - goto fail; - -return buf; +return 0 ; fail: -kfree (buf); -return NULL ; +return -EINVAL ;
why -EINVAL? should we not return result, as received from scsi_vpd_inquiry?
Quoted 8 lines
drivers/scsi/scsi.c +0
-0}EXPORT_SYMBOL_GPL (scsi_get_vpd_page);drivers/scsi/sd.c +0
-0
drivers/scsi/sd.c void sd_read_app_tag_own(struct scsi_disk *sdkp, unsigned char *buffer) · line 1869
Quoted 7 lines
drivers/scsi/sd.c +2
-1@@ -1864,19 +1864,20 @@ void sd_read_app_tag_own(struct scsi_disk *sdkp, unsigned char *buffer)static void sd_read_block_limits (struct scsi_disk *sdkp) {unsigned int sector_sz = sdkp->device->sector_size; -char *buffer; +const int vpd_len =32 ; +unsigned char *buffer =kmalloc (vpd_len,GFP_KERNEL );
32 sounds two small for me. Not because of the page but because of the first pass. Why not just 255 as a rule. We kmalloc it anyway.
We used to allocate 255 here, for some devices out there this is surly a regression.
Boaz
Quoted 90 lines
drivers/scsi/sd.c +13
-10-/* Block Limits VPD */ - buffer =scsi_get_vpd_page (sdkp->device,0xb0 ); - -if (buffer ==NULL ) -return ; +if (!buffer || +/* Block Limits VPD */ +scsi_get_vpd_page (sdkp->device,0xb0 , buffer, vpd_len)) + goto out;blk_queue_io_min (sdkp->disk->queue,get_unaligned_be16 (&buffer[6 ]) * sector_sz);blk_queue_io_opt (sdkp->disk->queue,get_unaligned_be32 (&buffer[12 ]) * sector_sz); + out:kfree (buffer); }@@ -1886,20 +1887,23 @@ static void sd_read_block_limits(struct scsi_disk *sdkp)*/ static void sd_read_block_characteristics (struct scsi_disk *sdkp) { -char *buffer; +unsigned char *buffer;u16 rot; +const int vpd_len =32 ; -/* Block Device Characteristics VPD */ - buffer =scsi_get_vpd_page (sdkp->device,0xb1 ); + buffer =kmalloc (vpd_len,GFP_KERNEL ); -if (buffer ==NULL ) -return ; +if (!buffer || +/* Block Device Characteristics VPD */ +scsi_get_vpd_page (sdkp->device,0xb1 , buffer, vpd_len)) + goto out; rot =get_unaligned_be16 (&buffer[4 ]);if (rot ==1 )queue_flag_set_unlocked (QUEUE_FLAG_NONROT , sdkp->disk->queue); + out:kfree (buffer); }drivers/scsi/ses.c +7
-3@@ -448,13 +448,17 @@ static void ses_match_to_enclosure(struct enclosure_device *edev, .addr = 0, }; - buf =scsi_get_vpd_page (sdev,0x83 ); -if (!buf) -return ; + buf =kmalloc (INIT_ALLOC_SIZE ,GFP_KERNEL ); +if (!buf ||scsi_get_vpd_page (sdev,0x83 , buf,INIT_ALLOC_SIZE )) + goto free;ses_enclosure_data_process (edev,to_scsi_device (edev->edev.parent),0 ); vpd_len = ((buf[2 ] <<8 ) | buf[3 ]) +4 ; +kfree (buf); + buf =kmalloc (vpd_len,GFP_KERNEL ); +if (!buf ||scsi_get_vpd_page (sdev,0x83 , buf, vpd_len)) + goto free; desc = buf +4 ;while (desc < buf + vpd_len) {include/scsi/scsi_device.h +2
-1@@ -347,7 +347,8 @@ extern int scsi_mode_select(struct scsi_device *sdev, int pf, int sp, struct scsi_sense_hdr *);extern int scsi_test_unit_ready (struct scsi_device *sdev,int timeout,int retries,struct scsi_sense_hdr *sshdr); -extern unsigned char *scsi_get_vpd_page (struct scsi_device *,u8 page); +extern int scsi_get_vpd_page (struct scsi_device *,u8 page,unsigned char *buf, +int buf_len);extern int scsi_device_set_state (struct scsi_device *sdev,enum scsi_device_state state); extern struct scsi_event *sdev_evt_alloc(enum scsi_device_event evt_type,
Quoted 51 lines · Boaz Harrosh
Quoted 50 lines · James Bottomley
Quoted 33 lines · James Bottomley
Quoted 26 lines · Boaz Harrosh
Quoted 20 lines · James Bottomley
Quoted 9 lines · Roel Kluin
kmalloc()may fail, so test whether it succeeded.Signed-off-by Roel Kluin <[email protected]>---
drivers/scsi/scsi.c +0
-0drivers/scsi/scsi.c unsigned char *scsi_get_vpd_page(struct scsi_device *sdev, u8 page) · line 1061
Quoted 6 lines
drivers/scsi/scsi.c +3
-0@@ -1056,6 +1056,9 @@ unsigned char *scsi_get_vpd_page(struct scsi_device *sdev, u8 page)kfree (buf); buf =kmalloc (len +4 ,GFP_KERNEL ); +if (!buf) +return NULL ; +Firstly, this won't actually apply ... you should be developing against either the SCSI trees or linux-next to get the latest versions in git.
Quoted 4 lines
Secondly it's not really right for the most common use cases, which don't usually want the whole vpd buffer anyway. I don't really see a simple way of fixing it without altering the interface, though.
Quoted 5 lines
"most common use cases" do not have pages bigger then 255. Can you think of any? For these few places that anticipate pages bigger then 255 I don't see much choice, we just freed 255 bytes and tried a new size, say 317, which failed with
GFP_KERNEL, In such a busy system, better fail with NULL then BUG, No? In any way caller must check for NULL because of the first allocation.Quoted 7 lines
Other way around: common use cases means the callers. What I'm saying is that regardless of the size of the VPD page, the caller usually only wants a few bytes into it. Once we have all the information the caller ever needs, there's no point in trying again with a larger buffer just because the VPD page is larger ... to code this into the interface, though, the caller would need to specify the max length it is looking for.
OK, it's been a couple of months and no patch has been forthcoming on this, so how about the attached? It does the query and only adjusts the size where the caller actually wants to bother.
James
---
Quoted 11 lines
drivers/scsi/scsi.c static int scsi_vpd_inquiry(struct scsi_device *sdev, unsigned char *buffer, · line 1028
Quoted 8 lines
drivers/scsi/scsi.c +0
-0@@ -1026,55 +1026,39 @@ static int scsi_vpd_inquiry(struct scsi_device *sdev, unsigned char *buffer,* responsible for calling kfree() on this pointer when it is no longer * needed. If we cannot retrieve the VPD page this routine returns %NULL. */ The comment is wrong now, I would also say something about
buf_lenis recommended to be 255 or more since other wise we might miss out on pages in the first inquiry.
Will fix.
Quoted 18 lines
Quoted 18 lines
drivers/scsi/scsi.c +3
-8-unsigned char *scsi_get_vpd_page (struct scsi_device *sdev,u8 page) +int scsi_get_vpd_page (struct scsi_device *sdev,u8 page,unsigned char *buf, +int buf_len) {int i, result; -unsigned int len; -const unsigned int init_vpd_len =255 ; -unsigned char *buf =kmalloc (init_vpd_len,GFP_KERNEL ); - -if (!buf) -return NULL ;/* Ask for all the pages supported by this device */ - result =scsi_vpd_inquiry (sdev, buf,0 , init_vpd_len); + result =scsi_vpd_inquiry (sdev, buf,0 , buf_len);if (result) goto fail;
Quoted 7 lines
drivers/scsi/scsi.c static int scsi_vpd_inquiry(struct scsi_device *sdev, unsigned char *buffer, · line 1042
Quoted 5 lines
drivers/scsi/scsi.c +1
-1/* If the user actually wanted this page, we can skip the rest */ if (page ==0 ) -return buf; +return -EINVAL ;Why
-EINVAL; Look at the comment above, return 0 would be better.
Bug in code (well spotted), will fix.
Quoted 4 lines
Quoted 4 lines
drivers/scsi/scsi.c +1
-1-for (i =0 ; i < buf[3 ]; i++) +for (i =0 ; i <min ((int )buf[3 ], buf_len -4 ); i++)if (buf[i +4 ] == page) goto found;
Quoted 8 lines
drivers/scsi/scsi.c static int scsi_vpd_inquiry(struct scsi_device *sdev, unsigned char *buffer, · line 1049
Quoted 4 lines
drivers/scsi/scsi.c +4
-0+ +if (i < buf[3 ] && i > buf_len) +/* ran off the end of the buffer, give us benefit of doubt */ + goto found;Some cheep devices are known to break when asked for pages they do not support better return an
-ETOOSMALLthe user can check for. (And the comment above should also take care of it)
OK, so I struggled with this. The reason for the behaviour is that only USB devices with incredibly small page lists are likely to exhibit the problem. Whereas the page lists in array type devices are likely to grow. I don't want to run into the situation that your new $10m array suddenly gives an error with DIF/DIX because the page list is too big and the problem this is checking for only afflicts cheap and badly implemented HW.
Quoted 26 lines
Quoted 26 lines
drivers/scsi/scsi.c +2
-16/* The device claims it doesn't support the requested page */ goto fail; found: - result =scsi_vpd_inquiry (sdev, buf, page,255 ); + result =scsi_vpd_inquiry (sdev, buf, page, buf_len);if (result) goto fail; -/* -* Some pages are longer than 255 bytes. The actual length of -* the page is returned in the header. -*/ - len = ((buf[2 ] <<8 ) | buf[3 ]) +4 ; -if (len <= init_vpd_len) -return buf; - -kfree (buf); - buf =kmalloc (len,GFP_KERNEL ); - result =scsi_vpd_inquiry (sdev, buf, page, len); -if (result) - goto fail; - -return buf; +return 0 ;
Quoted 6 lines
drivers/scsi/scsi.c static int scsi_vpd_inquiry(struct scsi_device *sdev, unsigned char *buffer, · line 1061
Quoted 4 lines
drivers/scsi/scsi.c +1
-2fail: -kfree (buf); -return NULL ; +return -EINVAL ;why
-EINVAL? should we not return result, as received fromscsi_vpd_inquiry?
Yes, was in two minds about that. Since zero is success, we can return result easily ... I just like the idea of negative error numbers.
Quoted 18 lines
Quoted 8 lines
drivers/scsi/scsi.c +0
-0}EXPORT_SYMBOL_GPL (scsi_get_vpd_page);drivers/scsi/sd.c +0
-0drivers/scsi/sd.c void sd_read_app_tag_own(struct scsi_disk *sdkp, unsigned char *buffer) · line 1869
Quoted 7 lines
drivers/scsi/sd.c +2
-1@@ -1864,19 +1864,20 @@ void sd_read_app_tag_own(struct scsi_disk *sdkp, unsigned char *buffer)static void sd_read_block_limits (struct scsi_disk *sdkp) {unsigned int sector_sz = sdkp->device->sector_size; -char *buffer; +const int vpd_len =32 ; +unsigned char *buffer =kmalloc (vpd_len,GFP_KERNEL );32 sounds two small for me. Not because of the page but because of the first pass. Why not just 255 as a rule. We kmalloc it anyway.
It only needs 16 if you look at the code. 32 is actually the smallest kmalloc slab on 32 bit systems.
Quoted 2 lines
We used to allocate 255 here, for some devices out there this is surly a regression.
We did 255 because we were concerned about space for the page list ... hopefully I answered that one above.
James
Quoted 10 lines · James Bottomley
Quoted 8 lines · Boaz Harrosh
drivers/scsi/scsi.c static int scsi_vpd_inquiry(struct scsi_device *sdev, unsigned char *buffer, · line 1049
Quoted 4 lines
drivers/scsi/scsi.c +4
-0+ +if (i < buf[3 ] && i > buf_len) +/* ran off the end of the buffer, give us benefit of doubt */ + goto found;Some cheep devices are known to break when asked for pages they do not support better return an
-ETOOSMALLthe user can check for. (And the comment above should also take care of it)
Quoted 8 lines
OK, so I struggled with this. The reason for the behaviour is that only USB devices with incredibly small page lists are likely to exhibit the problem. Whereas the page lists in array type devices are likely to grow. I don't want to run into the situation that your new $10m array suddenly gives an error with DIF/DIX because the page list is too big and the problem this is checking for only afflicts cheap and badly implemented HW.
If I remember the standard correctly and from inspecting the code 260 is the maximum size for page 0. (And the maximum we supported until today for page 0 was 256) so there is no speculations here. 256 it should be. If you want you can do an:
if (i < buf[3] && i > buf_len) {
if (likely(buf_len >= 255))
/* ran off the end of the buffer, give us benefit of doubt */
goto found;
else
return -ETOOSMALL;
}which is only theoretical because we only check against buf[3] that cannot be more then 255 so all is left is return -ETOOSMALL;
Callers that allocate smaller then 256 for this (hence the comment) must check for -ETOOSMALL;. Or should call with >= 256.
<snip>
Quoted 20 lines
Quoted 7 lines
Quoted 7 lines
drivers/scsi/sd.c +0
-0@@ -1864,19 +1864,20 @@ void sd_read_app_tag_own(struct scsi_disk *sdkp, unsigned char *buffer)static void sd_read_block_limits (struct scsi_disk *sdkp) {Quoted 8 lines
drivers/scsi/sd.c void sd_read_app_tag_own(struct scsi_disk *sdkp, unsigned char *buffer) · line 1869
Quoted 5 lines
drivers/scsi/sd.c +2
-1unsigned int sector_sz = sdkp->device->sector_size; -char *buffer; +const int vpd_len =32 ; +unsigned char *buffer =kmalloc (vpd_len,GFP_KERNEL );32 sounds two small for me. Not because of the page but because of the first pass. Why not just 255 as a rule. We kmalloc it anyway.
It only needs 16 if you look at the code. 32 is actually the smallest kmalloc slab on 32 bit systems.
Quoted 6 lines
Quoted 2 lines
We used to allocate 255 here, for some devices out there this is surly a regression.
We did 255 because we were concerned about space for the page list ... hopefully I answered that one above.
No, I still disagree. Whats the point of checking at all? Surly there is a device out there that has more then 12 pages but will breaks with wrong page. So what's the limit? the standards say 260 why not stick with that. It's not at any hot path.
Quoted 3 lines
James
Boaz
Quoted 20 lines · Boaz Harrosh
Quoted 10 lines · James Bottomley
Quoted 8 lines · Boaz Harrosh
drivers/scsi/scsi.c static int scsi_vpd_inquiry(struct scsi_device *sdev, unsigned char *buffer, · line 1049
Quoted 4 lines
drivers/scsi/scsi.c +4
-0+ +if (i < buf[3 ] && i > buf_len) +/* ran off the end of the buffer, give us benefit of doubt */ + goto found;Some cheep devices are known to break when asked for pages they do not support better return an
-ETOOSMALLthe user can check for. (And the comment above should also take care of it)Quoted 8 lines
OK, so I struggled with this. The reason for the behaviour is that only USB devices with incredibly small page lists are likely to exhibit the problem. Whereas the page lists in array type devices are likely to grow. I don't want to run into the situation that your new $10m array suddenly gives an error with DIF/DIX because the page list is too big and the problem this is checking for only afflicts cheap and badly implemented HW.
Quoted 4 lines
If I remember the standard correctly and from inspecting the code 260 is the maximum size for page 0. (And the maximum we supported until today for page 0 was 256) so there is no speculations here. 256 it should be. If you want you can do an:
The standard actually lists only two mandatory pages and about eight optional ones (plus the 127 vendor specific ones that no-one seems to use).
The point isn't to exercise the standard to the maximum, it's to generate working code. The point is that problem devices likely only list a couple of VPD pages, so we'll correctly not probe them. Conforming devices don't matter because they'll do the right thing when asked for a VPD page they don't have.
Quoted 38 lines
if (i < buf[3] && i > buf_len) { if (likely(buf_len >= 255)) /* ran off the end of the buffer, give us benefit of doubt */ goto found; else return -ETOOSMALL; }which is only theoretical because we only check against buf[3] that cannot be more then 255 so all is left is return
-ETOOSMALL;Callers that allocate smaller then 256 for this (hence the comment) must check for
-ETOOSMALL;. Or should call with >= 256.<snip>
Quoted 23 lines
Quoted 7 lines
Quoted 7 lines
drivers/scsi/sd.c +0
-0@@ -1864,19 +1864,20 @@ void sd_read_app_tag_own(struct scsi_disk *sdkp, unsigned char *buffer)static void sd_read_block_limits (struct scsi_disk *sdkp) {Quoted 8 lines
drivers/scsi/sd.c void sd_read_app_tag_own(struct scsi_disk *sdkp, unsigned char *buffer) · line 1869
Quoted 5 lines
drivers/scsi/sd.c +2
-1unsigned int sector_sz = sdkp->device->sector_size; -char *buffer; +const int vpd_len =32 ; +unsigned char *buffer =kmalloc (vpd_len,GFP_KERNEL );32 sounds two small for me. Not because of the page but because of the first pass. Why not just 255 as a rule. We kmalloc it anyway.
It only needs 16 if you look at the code. 32 is actually the smallest kmalloc slab on 32 bit systems.
Quoted 2 lines
We used to allocate 255 here, for some devices out there this is surly a regression.
Quoted 7 lines
Quoted 3 lines
We did 255 because we were concerned about space for the page list ... hopefully I answered that one above.
No, I still disagree. Whats the point of checking at all? Surly there is a device out there that has more then 12 pages but will breaks with wrong page. So what's the limit? the standards say 260 why not stick with that. It's not at any hot path.
The point of checking is not to send a VPD inquiry to USB devices that don't support it. These have a very limited range of supported VPD pages.
James
Quoted 6 lines · James Bottomley
On Wed, 2009-11-04 at 18:18 +0200, Boaz Harrosh wrote:
The point of checking is not to send a VPD inquiry to USB devices that don't support it. These have a very limited range of supported VPD pages.
OK thanks. But maybe just define a MIN_INQUIRY_SIZE instead of hard coded 32 everywhere, and use that. So in future if such a device is found we can easily change it.
(Never say never ;-))
Quoted 3 lines
James
Boaz
Quoted 11 lines · Boaz Harrosh
Quoted 6 lines · James Bottomley
On Wed, 2009-11-04 at 18:18 +0200, Boaz Harrosh wrote:
The point of checking is not to send a VPD inquiry to USB devices that don't support it. These have a very limited range of supported VPD pages.
OK thanks. But maybe just define a
MIN_INQUIRY_SIZEinstead of hard coded 32 everywhere, and use that. So in future if such a device is found we can easily change it.
So the minimum inquiry size would actually be 36 ... from scsi_scan.c ... and that's hard coded to a value too.
Quoted 1 line
(Never say never ;-))
I'm hoping that by the time USB devices get complex enough to need more than 28 VPD pages, they've actually discovered what conforming to the standards means.
James
Quoted 9 lines · James Bottomley
Quoted 8 lines · Boaz Harrosh
Quoted 6 lines · James Bottomley
On Wed, 2009-11-04 at 18:18 +0200, Boaz Harrosh wrote:
The point of checking is not to send a VPD inquiry to USB devices that don't support it. These have a very limited range of supported VPD pages.
Quoted 7 lines
Quoted 3 lines
OK thanks. But maybe just define a
MIN_INQUIRY_SIZEinstead of hard coded 32 everywhere, and use that. So in future if such a device is found we can easily change it.So the minimum inquiry size would actually be 36 ... from
scsi_scan.c... and that's hard coded to a value too.
two wrongs don't make a right ;-) But yes, sorry, that's a wrong name. I meant minimum-vpd-page-inquiry, a specifc constant for that function we are talking about.
Quoted 6 lines
Quoted 1 line
(Never say never ;-))
I'm hoping that by the time USB devices get complex enough to need more than 28 VPD pages, they've actually discovered what conforming to the standards means.
I don't understand what your saying. I'm making a simple and sane point. A new comer, from looking at the API, will say "Haa I only need 12 bytes lets set that". But no the buffer has dual purpose, one get the page I want, but zero check these crap USB devices. (And all those other subtle things) So you are actually going to argue that a patch should introduce an hard coded 32 unexplained, and refuse a constant that actually communicates the issue, which also covers our asses from the hopes we make. Because I sure hope so too, but I'm also 48 years old.
Quoted 3 lines
James
Boaz