netdev - Re: [PATCH] brcmfmac: Use standard SKB list accessors in brcmf_sdiod_sglist

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: <60f8f1c2-2b20-be60-e248-13dfcc58298d@broadcom.com>
Date:   Tue, 13 Nov 2018 22:07:09 +0100
From:   Arend van Spriel <arend.vanspriel@...adcom.com>
To:     Kalle Valo <kvalo@...eaurora.org>,
        David Miller <davem@...emloft.net>
Cc:     netdev@...r.kernel.org, linux-wireless@...r.kernel.org
Subject: Re: [PATCH] brcmfmac: Use standard SKB list accessors in
 brcmf_sdiod_sglist_rw.

On 11/13/2018 12:19 PM, Kalle Valo wrote:
> David Miller <davem@...emloft.net> writes:
>
>> [ As I am trying to remove direct SKB list pointer accesses I am
>>   committing this to net-next.  If this causes a lot of grief I
>>   can and will revert, just let me know. ]
>>
>> Instead of direct SKB list pointer accesses.
>>
>> The loops in this function had to be rewritten to accommodate this
>> more easily.
>>
>> The first loop iterates now over the target list in the outer loop,
>> and triggers an mmc data operation when the per-operation limits are
>> hit.
>>
>> Then after the loops, if we have any residue, we trigger the last
>> and final operation.
>>
>> For the page aligned workaround, where we have to copy the read data
>> back into the original list of SKBs, we use a two-tiered loop.  The
>> outer loop stays the same and iterates over pktlist, and then we have
>> an inner loop which uses skb_peek_next().  The break logic has been
>> simplified because we know that the aggregate length of the SKBs in
>> the source and destination lists are the same.
>>
>> This change also ends up fixing a bug, having to do with the
>> maintainance of the seg_sz variable and how it drove the outermost
>> loop.  It begins as:
>>
>> 	seg_sz = target_list->qlen;
>>
>> ie. the number of packets in the target_list queue.  The loop
>> structure was then:
>>
>> 	while (seq_sz) {
>> 		...
>> 		while (not at end of target_list) {
>> 			...
>> 			sg_cnt++
>> 			...
>> 		}
>> 		...
>> 		seg_sz -= sg_cnt;
>>
>> The assumption built into that last statement is that sg_cnt counts
>> how many packets from target_list have been fully processed by the
>> inner loop.  But this not true.
>>
>> If we hit one of the limits, such as the max segment size or the max
>> request size, we will break and copy a partial packet then contine
>> back up to the top of the outermost loop.
>>
>> With the new loops we don't have this problem as we don't guard the
>> loop exit with a packet count, but instead use the progression of the
>> pkt_next SKB through the list to the end.  The general structure is:
>>
>> 	sg_cnt = 0;
>> 	skb_queue_walk(target_list, pkt_next) {
>> 		pkt_offset = 0;
>> 		...
>> 		sg_cnt++;
>> 		...
>> 		while (pkt_offset < pkt_next->len) {
>> 			pkt_offset += sg_data_size;
>> 			if (queued up max per request)
>> 				mmc_submit_one();
>> 		}
>> 	}
>> 	if (sg_cnt)
>> 		mmc_submit_one();
>>
>> The variables that maintain where we are in the MMC command state such
>> as req_sz, sg_cnt, and sgl are reset when we emit one of these full
>> sized requests.
>>
>> Signed-off-by: David S. Miller <davem@...emloft.net>
>
> Looks good to me, thanks.

Looks good to me too. However, I currently do not have the hardware at 
hands to give it a run for its money. I would prefer to have a tested-by 
tag. May take me a couple of days to revive a setup.

Regards,
Arend