netdev - Re: [PATCH V2 0/4] Add IPv6 support for iWARP

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <51C3058D.205@chelsio.com>
Date:	Thu, 20 Jun 2013 19:07:17 +0530
From:	Vipul Pandya <vipul@...lsio.com>
To:	David Miller <davem@...emloft.net>
CC:	SWise OGC <swise@...ngridcomputing.com>,
	"linux-rdma@...r.kernel.org" <linux-rdma@...r.kernel.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"roland@...estorage.com" <roland@...estorage.com>,
	Divy Le Ray <divy@...lsio.com>,
	Dimitrios Michailidis <dm@...lsio.com>,
	"roland@...nel.org" <roland@...nel.org>,
	"sean.hefty@...el.com" <sean.hefty@...el.com>,
	"hal.rosenstock@...il.com" <hal.rosenstock@...il.com>,
	Tom Tucker <tom@...ngridcomputing.com>,
	"faisal.latif@...el.com" <faisal.latif@...el.com>,
	"akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
	"sasha.levin@...cle.com" <sasha.levin@...cle.com>,
	Nirranjan Kirubaharan <nirranjan@...lsio.com>
Subject: Re: [PATCH V2 0/4] Add IPv6 support for iWARP



On 20-06-2013 09:38, David Miller wrote:
> From: Steve Wise <swise@...ngridcomputing.com>
> Date: Wed, 19 Jun 2013 21:19:13 -0500
> 
>> On 6/19/2013 8:01 PM, David Miller wrote:
>>> From: Vipul Pandya <vipul@...lsio.com>
>>> Date: Wed, 12 Jun 2013 17:11:38 +0530
>>>
>>>> We have included all the maintainers of respective drivers. Kindly
>>>> review the change and let us know in case of any review comments.
>>> I have not seen anyone review v2 of this patch series.
>>>
>>
>> Reviewed-by: Steve Wise <swise@...ngridcomputing.com>
> 
> You wrote the first patch, and I bet you didn't even read the code in
> the cxgb4 driver.  So your review is sort of pointless... UNLESS you
> spotted the obvious bugs in these changes, that would have been
> interesting.
> 
> Because NOBODY, and I mean NOBODY, even looked at the build of the
> cxgb4 changes.
> 
> Tell me what this does:
> 
>  	struct tid_info *t = dev->rdev.lldi.tids;
>  	int status = GET_AOPEN_STATUS(ntohl(rpl->atid_status));
> +	struct sockaddr_in *la = (struct sockaddr_in *)&ep->com.local_addr;
> +	struct sockaddr_in *ra = (struct sockaddr_in *)&ep->com.remote_addr;
> +	struct sockaddr_in6 *la6 = (struct sockaddr_in6 *)&ep->com.local_addr;
> +	struct sockaddr_in6 *ra6 = (struct sockaddr_in6 *)&ep->com.remote_addr;
> +
> +
>  
>  	ep = lookup_atid(t, atid);
> 
> Dereferencing 'ep' before initializing it.
> 
> The compiler complains loudly about this, therefore nobody even looked at
> the build logs from these changes before submitting them to me.
> 
> That translates to "don't care", and if the people submitting this
> code don't care why should I?
> 
> Sorry, not impressed.  I'm seriously going to take my time reviewing
> any future submissions of these changes, because it's obvious that
> even the people writing and submitting this code DO NOT CARE.
> 

I am really very sorry for this. Somehow my compiler is not giving me
any warnings for this. My compiler is gcc 4.4.6 20120305 (Red Hat
4.4.6-4). Previously also once it has happened that my compiler did not
give any warning but your build environment caught. Is there any special
gcc option I have to pass with make command for this? I am following the
checklist mentioned in Documentation/SubmitChecklist file.

We always make sure all our drivers are building cleanly before
submitting the drivers. We also have unit tested this code. However the
problematic code gets executed only in error path hence could not catch
during unit testing.

I will resubmitt the series with the changes. Your review comments are
very valuable for us.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html