LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: James Bottomley <James.Bottomley@HansenPartnership.com>
To: Yinghai Lu <Yinghai.Lu@Sun.COM>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	linux-scsi@vger.kernel.org, linux-ide@vger.kernel.org,
	kristen.c.accardi@intel.com
Subject: Re: [PATCH] scsi: ses fix for len and mem leaking when fail to add intf
Date: Sat, 09 Feb 2008 09:00:42 -0600	[thread overview]
Message-ID: <1202569242.4254.9.camel@localhost.localdomain> (raw)
In-Reply-To: <200802090413.53275.yinghai.lu@sun.com>


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.

> 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.

>  	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).

>  	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).

>  	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.
 
>  	/* 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.

> -					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.

>  	kfree(ses_dev->page10);
>  	kfree(ses_dev->page2);
>  	kfree(ses_dev->page1);
> @@ -630,6 +636,7 @@ static void ses_intf_remove(struct class
>  	ses_dev = edev->scratch;
>  	edev->scratch = NULL;
>  
> +	kfree(ses_dev->page10);

Yes, a bug, thanks.

>  	kfree(ses_dev->page1);
>  	kfree(ses_dev->page2);
>  	kfree(ses_dev);

James



  reply	other threads:[~2008-02-09 15:01 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-02-09 12:13 Yinghai Lu
2008-02-09 15:00 ` James Bottomley [this message]
2008-02-09 22:28   ` Yinghai Lu
2008-02-09 23:15   ` [PATCH] scsi: ses fix " Yinghai Lu
2008-02-11  4:28     ` James Bottomley
2008-02-11  5:27       ` Yinghai Lu
2008-02-11  7:25       ` [SCSI] ses: fix memory leaks Yinghai Lu
2008-02-11 16:23         ` James Bottomley
2008-02-11 17:02           ` James Bottomley
2008-02-11 20:25             ` Yinghai Lu
2008-02-13  7:10               ` [PATCH] SCSI: fix data corruption caused by ses Yinghai Lu
2008-02-13 23:25                 ` James Bottomley
2008-02-14  0:07                   ` Yinghai Lu
2008-02-14  0:25                   ` [PATCH] SCSI: fix data corruption caused by ses v2 Yinghai Lu
2008-02-15 15:53                     ` James Bottomley
2008-02-15 18:44                       ` Yinghai Lu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1202569242.4254.9.camel@localhost.localdomain \
    --to=james.bottomley@hansenpartnership.com \
    --cc=Yinghai.Lu@Sun.COM \
    --cc=akpm@linux-foundation.org \
    --cc=kristen.c.accardi@intel.com \
    --cc=linux-ide@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-scsi@vger.kernel.org \
    --subject='Re: [PATCH] scsi: ses fix for len and mem leaking when fail to add intf' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).