netdev - Re: [PATCH] solos-pci: don't call vcc->pop() after pclose()

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:	Thu, 29 Nov 2012 14:20:21 +0100
From:	Krzysztof Mazur <krzysiek@...lesie.net>
To:	David Woodhouse <dwmw2@...radead.org>
Cc:	David Laight <David.Laight@...LAB.COM>,
	chas williams - CONTRACTOR <chas@....nrl.navy.mil>,
	davem@...emloft.net, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org, nathan@...verse.com.au
Subject: Re: [PATCH] solos-pci: don't call vcc->pop() after pclose()

On Thu, Nov 29, 2012 at 12:57:17PM +0000, David Woodhouse wrote:
> On Thu, 2012-11-29 at 13:43 +0100, Krzysztof Mazur wrote:
> > 
> > Removing packets from tx_queue is not needed. We can transmit packets
> > also after close. We just can't call vcc->pop() after close,
> > so we can just set SKB_CB(skb)->vcc of such packets to NULL so
> > fpga_tx() won't call vcc->pop().
> 
> Your patch doesn't do that, does it? You'd want something like
> 
>  if (card->tx_skb[port] && SKB_CB(card->tx_skb[port]->vcc) == vcc)
>     SKB_CB(card->tx_skb[port]->vcc) = NULL;

No, I want to process all queued packets, not just only those that
were already sent do card.

In that case we will need to remove other packets with that vcc
from queue, of couse we can still do that in the same loop, something
like:

	if (SKB_CB(skb)->vcc == vcc) {
		if (card->tx_skb[port] == skb) {
			skb_get(skb);
			solos_pop(SKB_CB(skb)->vcc, skb);
			SKB_CB(skb)->vcc = NULL;
		} else {
			skb_unlink(skb, &card->tx_queue[port]);
			solos_pop(SKB_CB(skb)->vcc, skb);
		}
	}

But I don't think that this optization is needed.

> 
> Under card->tx_lock should suffice.
> 
> And do we just *not* call the ->pop() on that skb ever? And hope that it
> doesn't screw up some other state somewhere? Like if we're doing MLPPP
> and I've implemented BQL for PPP... we might never call
> ppp_completed_queue() for that skb, so even though this *channel* is
> going away, it might still contribute towards the perceived queue on the
> overall PPP netdev?
> 
> Failing to call ->pop() could cause memory leaks and other issues; I
> don't think it's reasonable. I think we *have* to wait for
> card->tx_skb[port] if it's for the VCC we're closing.

We are calling ->pop() in solos_pop() just before SKB_CB(skb)->vcc = NULL,
but we are doing that before we really finish processing that packet,
that's why we do skb_get(skb).

Krzysiek
--
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