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]

Date:   Tue, 13 Nov 2018 13:19:22 +0200
From:   Kalle Valo <kvalo@...eaurora.org>
To:     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.

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.

Acked-by: Kalle Valo <kvalo@...eaurora.org>

-- 
Kalle Valo