On Sun, 2013-11-17 at 23:12 -0200, Geyslan Gregório Bem wrote: > 2013/11/17 James Bottomley <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx>: > > On Sun, 2013-11-17 at 19:09 -0200, Geyslan Gregório Bem wrote: > >> 2013/11/17 James Bottomley <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx>: > >> > On Sun, 2013-11-17 at 15:51 -0300, Geyslan G. Bem wrote: > >> >> This patch fix memory leakage in cases 'ISCSI_NET_PARAM_VLAN_ID' and > >> >> 'ISCSI_NET_PARAM_VLAN_PRIORITY' and refactors code 'going out' when > >> >> necessary. > >> > > >> > You pointlessly renamed a variable, which makes the diff hard to read. > >> > Please don't do that. > >> > >> Ok, I can agree. 'len' means length? What is returned in case of non > >> error? > > > > it returns the length of buf written to or negative error. > > > >> > You missed the fact that the passed in pointer is unmodified if > >> > mgmt_get_if_info() returns non zero, so the kfree frees junk and would > >> > oops. > >> > > >> > There's no need for a goto; len = -EINVAL; does everything that's > >> > needed. > >> > >> Well, that is a coverity catch. CID 1128954. Check it. > > > > I didn't say coverity was wrong, I said your patch was (well not wrong, > > just over complex and incomplete). This is the way to fix both > > problems. > > > > James > > > > --- > > > > diff --git a/drivers/scsi/be2iscsi/be_iscsi.c b/drivers/scsi/be2iscsi/be_iscsi.c > > index ffadbee..9dcbdfa 100644 > > --- a/drivers/scsi/be2iscsi/be_iscsi.c > > +++ b/drivers/scsi/be2iscsi/be_iscsi.c > > @@ -541,10 +541,8 @@ static int be2iscsi_get_if_param(struct beiscsi_hba *phba, > > ip_type = BE2_IPV6; > > James, this approach will not prevent the leakage. I don't see why not. The -EINVAL case goes through the kfree() now too, no? > We can initialize the if_info with NULL and always kfree it without > to care about junk. Why? Error return means no allocation. James -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
