On Feb 9, 2008 7:00 AM, James Bottomley <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx> wrote: > > On Sat, 2008-02-09 at 04:13 -0800, Yinghai Lu wrote: > > [PATCH] scsi: ses fix for len and mem leaking when fail to add intf > > > > change to u32 before left shifting char > > This one is a bit unnecessary; C promotion rules guarantee that > everything is promoted to int (or above) before doing arithmetic. Since > it's only ever done on 16 bits, signed or unsigned int is adequate for > the conversion. thank. just learned that. yhlu@yhlunb:~/xx/xx/notes> cat ctest.c #include <stdio.h> int main(int argc, char *argv[]) { unsigned char buf[20]; int len; buf[2] = 0x02; buf[3] = 0x03; len = (buf[2] << 8) + buf[3]; printf("len = %x\n", len); return 0; } yhlu@yhlunb:~/xx/xx/notes> gcc -o ctest ctest.c yhlu@yhlunb:~/xx/xx/notes> ./ctest len = 203 yhlu@yhlunb:~/xx/xx/notes> > > > also fix leaking with scomp leaking when failing. > > Yes, I see that, thanks! There's also the kmalloc of scomp which should > be kzalloc if you care to fix that up in the resend. > > > - edev = enclosure_find(&sdev->host->shost_gendev); > > + edev = enclosure_find(&sdev->host->shost_gendev); > > Space cleanups also need mention in the changelog. > > > - ses_dev->page1 = buf; > > - ses_dev->page1_len = len; > > - > > result = ses_recv_diag(sdev, 1, buf, len); > > if (result) > > goto recv_failed; > > > > + ses_dev->page1 = buf; > > + ses_dev->page1_len = len; > > + > > Neither of us gets this right. By removing the kfree(buf) from the > err_free path, you cause a leak here. I cause a double free. I think > putting back the kfree(buf) and keeping this hunk is the fix. the buf already become sdev->page1, sdev->pag10, sdev->page2. so it will be freed via them > > > types = buf[10]; > > len = buf[11]; > > > > @@ -474,11 +474,12 @@ static int ses_intf_add(struct class_dev > > components += type_ptr[1]; > > } > > > > + buf = NULL; > > Yes, prevents double free (but only if buf is freed). it became sdev->page1 already > > > result = ses_recv_diag(sdev, 2, hdr_buf, INIT_ALLOC_SIZE); > > if (result) > > goto recv_failed; > > > > @@ -492,11 +493,12 @@ static int ses_intf_add(struct class_dev > > > > /* The additional information page --- allows us > > * to match up the devices */ > > + buf = NULL; > > It's probably better to move these closer to the statements that make > them necessary (in this case above the comment). OK > > > if (IS_ERR(edev)) { > > err = PTR_ERR(edev); > > + kfree(scomp); > > goto err_free; > > } > > kfree(scomp) should be in the err_free path just in case someone else > adds something to this. ok. > > > /* add 1 for trailing '\0' we'll use */ > > buf = kzalloc(len + 1, GFP_KERNEL); > > - result = ses_recv_diag(sdev, 7, buf, len); > > - if (result) { > > + if (buf) > > + result = ses_recv_diag(sdev, 7, buf, len); > > + else > > + result = 7; > > + > > What exactly is this supposed to be doing, and why 7? If you're > thinking of conditioning the page 7 receive on the success of the > allocation, we really need the allocation failure report more than we > need the driver to attach. want to move out label out of if later. > > > - addl_desc_ptr += addl_desc_ptr[1] + 2; > > + addl_desc_ptr += 2 + addl_desc_ptr[1]; > > This is rather pointless, isn't it? > > > err_free: > > - kfree(buf); > > You can't remove this. Also add kfree(scomp) here. - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html