netdev - Re: [PATCH net-next 5/6] ionic: implement support for rx sgl

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, 22 Oct 2019 17:14:11 -0700
From:   Shannon Nelson <snelson@...sando.io>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>
Cc:     netdev@...r.kernel.org, davem@...emloft.net
Subject: Re: [PATCH net-next 5/6] ionic: implement support for rx sgl

On 10/22/19 4:37 PM, Jakub Kicinski wrote:
> On Tue, 22 Oct 2019 13:31:12 -0700, Shannon Nelson wrote:
>> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_txrx.c b/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
>> index ab6663d94f42..8c96f5fe43a2 100644
>> --- a/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
>> +++ b/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
>> @@ -34,52 +34,107 @@ static inline struct netdev_queue *q_to_ndq(struct ionic_queue *q)
>>   	return netdev_get_tx_queue(q->lif->netdev, q->index);
>>   }
>>   
>> -static void ionic_rx_recycle(struct ionic_queue *q, struct ionic_desc_info *desc_info,
>> -			     struct sk_buff *skb)
>> +static struct sk_buff *ionic_rx_skb_alloc(struct ionic_queue *q,
>> +					  unsigned int len, bool frags)
>>   {
>> -	struct ionic_rxq_desc *old = desc_info->desc;
>> -	struct ionic_rxq_desc *new = q->head->desc;
>> +	struct ionic_lif *lif = q->lif;
>> +	struct ionic_rx_stats *stats;
>> +	struct net_device *netdev;
>> +	struct sk_buff *skb;
>> +
>> +	netdev = lif->netdev;
>> +	stats = q_to_rx_stats(q);
>>   
>> -	new->addr = old->addr;
>> -	new->len = old->len;
>> +	if (frags)
>> +		skb = napi_get_frags(&q_to_qcq(q)->napi);
>> +	else
>> +		skb = netdev_alloc_skb_ip_align(netdev, len);
>>   
>> -	ionic_rxq_post(q, true, ionic_rx_clean, skb);
>> +	if (unlikely(!skb)) {
>> +		net_warn_ratelimited("%s: SKB alloc failed on %s!\n",
>> +				     netdev->name, q->name);
>> +		stats->alloc_err++;
>> +		return NULL;
>> +	}
>> +
>> +	return skb;
>>   }
>>   
>> -static bool ionic_rx_copybreak(struct ionic_queue *q, struct ionic_desc_info *desc_info,
>> -			       struct ionic_cq_info *cq_info, struct sk_buff **skb)
>> +static struct sk_buff *ionic_rx_frags(struct ionic_queue *q,
>> +				      struct ionic_desc_info *desc_info,
>> +				      struct ionic_cq_info *cq_info)
>>   {
>>   	struct ionic_rxq_comp *comp = cq_info->cq_desc;
>> -	struct ionic_rxq_desc *desc = desc_info->desc;
>> -	struct net_device *netdev = q->lif->netdev;
>>   	struct device *dev = q->lif->ionic->dev;
>> -	struct sk_buff *new_skb;
>> -	u16 clen, dlen;
>> -
>> -	clen = le16_to_cpu(comp->len);
>> -	dlen = le16_to_cpu(desc->len);
>> -	if (clen > q->lif->rx_copybreak) {
>> -		dma_unmap_single(dev, (dma_addr_t)le64_to_cpu(desc->addr),
>> -				 dlen, DMA_FROM_DEVICE);
>> -		return false;
>> -	}
>> +	struct ionic_page_info *page_info;
>> +	struct sk_buff *skb;
>> +	unsigned int i;
>> +	u16 frag_len;
>> +	u16 len;
>>   
>> -	new_skb = netdev_alloc_skb_ip_align(netdev, clen);
>> -	if (!new_skb) {
>> -		dma_unmap_single(dev, (dma_addr_t)le64_to_cpu(desc->addr),
>> -				 dlen, DMA_FROM_DEVICE);
>> -		return false;
>> -	}
>> +	page_info = &desc_info->pages[0];
>> +	len = le16_to_cpu(comp->len);
>>   
>> -	dma_sync_single_for_cpu(dev, (dma_addr_t)le64_to_cpu(desc->addr),
>> -				clen, DMA_FROM_DEVICE);
>> +	prefetch(page_address(page_info->page) + NET_IP_ALIGN);
>>   
>> -	memcpy(new_skb->data, (*skb)->data, clen);
>> +	skb = ionic_rx_skb_alloc(q, len, true);
>> +	if (unlikely(!skb))
>> +		return NULL;
>>   
>> -	ionic_rx_recycle(q, desc_info, *skb);
>> -	*skb = new_skb;
>> +	i = comp->num_sg_elems + 1;
>> +	do {
>> +		if (unlikely(!page_info->page)) {
>> +			dev_kfree_skb(skb);
>> +			return NULL;
>> +		}
> Would you not potentially free the napi->skb here? is that okay?

Yes, probably a good idea.  I'll update that.

sln