LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: "Yinghai Lu" <yhlu.kernel@gmail.com>
To: "James Bottomley" <James.Bottomley@hansenpartnership.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, 9 Feb 2008 14:28:46 -0800	[thread overview]
Message-ID: <86802c440802091428v87a10f5h280ae1d82b1db6a@mail.gmail.com> (raw)
In-Reply-To: <1202569242.4254.9.camel@localhost.localdomain>

On Feb 9, 2008 7:00 AM, James Bottomley
<James.Bottomley@hansenpartnership.com> 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.

  reply	other threads:[~2008-02-09 22:29 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
2008-02-09 22:28   ` Yinghai Lu [this message]
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=86802c440802091428v87a10f5h280ae1d82b1db6a@mail.gmail.com \
    --to=yhlu.kernel@gmail.com \
    --cc=James.Bottomley@hansenpartnership.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).