netdev - Re: [PATCH v2 net] staging: octeon: Drop on uncorrectable alignment or FCS error

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: <93d7a7a7-69f9-6452-aa2f-2f82636ecde2@nokia.com>
Date:   Mon, 19 Oct 2020 11:11:01 +0200
From:   Alexander Sverdlin <alexander.sverdlin@...ia.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     devel@...verdev.osuosl.org,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Aaro Koskinen <aaro.koskinen@....fi>,
        Ralf Baechle <ralf@...ux-mips.org>,
        "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org
Subject: Re: [PATCH v2 net] staging: octeon: Drop on uncorrectable alignment
 or FCS error

Hello Andrew,

thank you for your review!

On 17/10/2020 23:02, Andrew Lunn wrote:
>> diff --git a/drivers/staging/octeon/ethernet-rx.c b/drivers/staging/octeon/ethernet-rx.c
>> index 2c16230..9ebd665 100644
>> --- a/drivers/staging/octeon/ethernet-rx.c
>> +++ b/drivers/staging/octeon/ethernet-rx.c
>> @@ -69,15 +69,17 @@ static inline int cvm_oct_check_rcv_error(struct cvmx_wqe *work)
>>  	else
>>  		port = work->word1.cn38xx.ipprt;
>>  
>> -	if ((work->word2.snoip.err_code == 10) && (work->word1.len <= 64)) {
>> +	if ((work->word2.snoip.err_code == 10) && (work->word1.len <= 64))
> It would be nice to replace all these err_code magic numbers with #defines.
> 
> You should also replace 64 with ETH_ZLEN + ETH_FCS_LEN. I also wonder
> if <= should be just < ?

I think all your comments are valid points, but are rather topics
for separate patches. In this one I've addressed one issue: the structure of
ifs and elses is so deeply nested, that it lead to one logic mistake:
broken packets which cannot be corrected are still not dropped.

Even my patch has two changes in one: error correction and style correction,
but I consider this justified because one was a result of another.

>>  		/*
>>  		 * Ignore length errors on min size packets. Some
>>  		 * equipment incorrectly pads packets to 64+4FCS
>>  		 * instead of 60+4FCS.  Note these packets still get
>>  		 * counted as frame errors.
>>  		 */
>> -	} else if (work->word2.snoip.err_code == 5 ||
>> -		   work->word2.snoip.err_code == 7) {
>> +		return 0;
>> +
>> +	if (work->word2.snoip.err_code == 5 ||
>> +	    work->word2.snoip.err_code == 7) {
>>  		/*
>>  		 * We received a packet with either an alignment error
>>  		 * or a FCS error. This may be signalling that we are
>> @@ -108,7 +110,10 @@ static inline int cvm_oct_check_rcv_error(struct cvmx_wqe *work)
>>  				/* Port received 0xd5 preamble */
>>  				work->packet_ptr.s.addr += i + 1;
>>  				work->word1.len -= i + 5;
>> -			} else if ((*ptr & 0xf) == 0xd) {
>> +				return 0;
>> +			}
>> +
>> +			if ((*ptr & 0xf) == 0xd) {
> The comments are not so clear what is going on here. Can this
> incorrectly match a destination MAC address of xD:XX:XX:XX:XX:XX.
> 
>>  				/* Port received 0xd preamble */
>>  				work->packet_ptr.s.addr += i;
>>  				work->word1.len -= i + 4;

-- 
Best regards,
Alexander Sverdlin.