On Fri, 2012-11-30 at 18:00 +0100, Krzysztof Mazur wrote:
> On Fri, Nov 30, 2012 at 04:23:46PM +, David Woodhouse wrote:
> >
> > +static void br2684_release_cb(struct atm_vcc *atmvcc)
> > +{
> > + struct br2684_vcc *brvcc = BR2684_VCC(atmvcc);
> > +
> > + /*
> > +* A race with br2684_x
On Fri, Nov 30, 2012 at 12:12:56PM -0500, chas williams - CONTRACTOR wrote:
> >Really, what we're saying is that *one* of the driver or protocol close
> >functions needs to be split, and we need to do DPD or PDP. Since the
> >device driver *can* abort/flush the TX queue and also any pending RX
> >b
On Fri, 30 Nov 2012 16:23:46 +
David Woodhouse wrote:
> On Fri, 2012-11-30 at 12:10 +, David Woodhouse wrote:
> > In that case I think we're fine. I'll just do the same thing in
> > br2684_push(), fix up the comment you just corrected, and we're all
> > good.
>
> OK, here's an update to
On Fri, Nov 30, 2012 at 04:23:46PM +, David Woodhouse wrote:
>
> +static void br2684_release_cb(struct atm_vcc *atmvcc)
> +{
> + struct br2684_vcc *brvcc = BR2684_VCC(atmvcc);
> +
> + /*
> + * A race with br2684_xmit_vcc() might cause a spurious wakeup just
> + * after that f
On Fri, 2012-11-30 at 12:10 +, David Woodhouse wrote:
> In that case I think we're fine. I'll just do the same thing in
> br2684_push(), fix up the comment you just corrected, and we're all
> good.
OK, here's an update to me my patch 8/17 'br2684: don't send frames on
not-ready vcc'. It takes
On Fri, 2012-11-30 at 10:53 +0100, Krzysztof Mazur wrote:
> On Fri, Nov 30, 2012 at 08:25:22AM +, David Woodhouse wrote:
> > On Fri, 2012-11-30 at 01:57 +, David Woodhouse wrote:
> > > I think it's actually fixed for pppoatm by the bh_lock_sock() and the
> > > sock_owned_by_user() check. As
On Fri, Nov 30, 2012 at 08:25:22AM +, David Woodhouse wrote:
> On Fri, 2012-11-30 at 01:57 +, David Woodhouse wrote:
> > I think it's actually fixed for pppoatm by the bh_lock_sock() and the
> > sock_owned_by_user() check. As soon as vcc_release() calls lock_sock(),
> > pppoatm stops accept
On Fri, 2012-11-30 at 01:57 +, David Woodhouse wrote:
> I think it's actually fixed for pppoatm by the bh_lock_sock() and the
> sock_owned_by_user() check. As soon as vcc_release() calls lock_sock(),
> pppoatm stops accepting packets.
>
> It should be simple enough to do the same in br2684.
U
On Thu, 2012-11-29 at 20:38 -0500, Chas Williams (CONTRACTOR) wrote:
> it isnt clear to me that fixes the race entirely either.
> vcc_destroy_socket() and any of the push()/sends()'s are not
> serialized.
> while you may clear the ATM_VF_READY flag, you might not clear it soon
> enough for any part
In message <1354227428.21562.230.ca...@shinybook.infradead.org>,David Woodhouse
writes:
>At this point, I think we're better off as we are (with Krzysztof's
>patch 1/7 dropped, and leaving vcc->dev->ops->close() being called
>before vcc->push(NULL). We've fairly much solved the issues with that
>a
On Thu, 2012-11-29 at 13:29 -0500, chas williams - CONTRACTOR wrote:
> On Thu, 29 Nov 2012 18:11:48 +
> David Woodhouse wrote:
>
> > We do see the 'packet received for unknown VCC' complaint, after we
> > reboot the host without resetting the card. And as shown in the commit I
> > just refere
On Thu, 29 Nov 2012 18:11:48 +
David Woodhouse wrote:
> We do see the 'packet received for unknown VCC' complaint, after we
> reboot the host without resetting the card. And as shown in the commit I
> just referenced, we rely on the !ATM_VF_READY check in order to prevent
> a use-after-free w
On Thu, 2012-11-29 at 12:17 -0500, chas williams - CONTRACTOR wrote:
> most atm hardware that i am familiar with, wont deliver vpi/vci data
> unless you are actually trying to receive it. however, this hardware
> is generally doing its reassembly in hardware and delivering aal5
> pdu's and needs t
On Thu, 29 Nov 2012 16:24:29 +
David Woodhouse wrote:
> On Thu, 2012-11-29 at 10:59 -0500, chas williams - CONTRACTOR wrote:
> > the part that bothers me (and i dont have the programmer's guide for
> > the solos hardware) is that you are watching for the PKT_PCLOSE to be
> > sent to the card.
On Thu, Nov 29, 2012 at 03:47:57PM +, David Woodhouse wrote:
> On Thu, 2012-11-29 at 16:09 +0100, Krzysztof Mazur wrote:
> >
> > I don't like two thinks about this patch:
> >
> > - if allos_skb(sizeof(*header), GFP_ATOMIC) at beginning of
> > pclose() fails we will crash
> >
On Thu, 2012-11-29 at 10:59 -0500, chas williams - CONTRACTOR wrote:
> the part that bothers me (and i dont have the programmer's guide for
> the solos hardware) is that you are watching for the PKT_PCLOSE to be
> sent to the card. shouldnt you be watching for the PKT_PCLOSE to be
> returned from
On Thu, 29 Nov 2012 15:59:08 +
David Woodhouse wrote:
> On Thu, 2012-11-29 at 10:37 -0500, chas williams - CONTRACTOR wrote:
> > you shouldnt clear ATM_VF_ADDR until the vpi/vci is actually closed and
> > ready for reuse. at this point, it isnt.
>
> So I should always wait for the completio
On Thu, 29 Nov 2012 15:47:57 +
David Woodhouse wrote:
> @@ -1020,12 +1048,15 @@ static uint32_t fpga_tx(struct solos_card *card)
> if (vcc) {
> atomic_inc(&vcc->stats->tx);
> solos_pop(vcc, oldskb);
> -
On Thu, 2012-11-29 at 10:37 -0500, chas williams - CONTRACTOR wrote:
> you shouldnt clear ATM_VF_ADDR until the vpi/vci is actually closed and
> ready for reuse. at this point, it isnt.
So I should always wait for the completion of my PKT_CLOSE and only
clear ATM_VF_ADDR when it's actually done?
On Thu, 2012-11-29 at 16:09 +0100, Krzysztof Mazur wrote:
>
> I don't like two thinks about this patch:
>
> - if allos_skb(sizeof(*header), GFP_ATOMIC) at beginning of
> pclose() fails we will crash
>
> - if card wakes up after this timeout we will probably crash too
>
On Wed, 28 Nov 2012 22:18:35 +
David Woodhouse wrote:
> diff --git a/drivers/atm/solos-pci.c b/drivers/atm/solos-pci.c
> index 9851093..3720670 100644
> --- a/drivers/atm/solos-pci.c
> +++ b/drivers/atm/solos-pci.c
> @@ -92,6 +92,7 @@ struct pkt_hdr {
> };
>
> struct solos_skb_cb {
> +
On Wed, Nov 28, 2012 at 10:18:35PM +, David Woodhouse wrote:
> On Wed, 2012-11-28 at 09:21 +, David Laight wrote:
> > Even when it might make sense to sleep in close until tx drains
> > there needs to be a finite timeout before it become abortive.
>
> You are, of course, right. We should n
On Thu, 29 Nov 2012 11:57:15 +0100
Krzysztof Mazur wrote:
> or if we really want to call vcc->pop() for such skbs:
you need to call ->pop() to cleaning up the wmem_alloc accounting.
otherwise you will get complaints from the atm stack later about
freeing a vcc that had outstanding data.
--
To un
On Thu, 2012-11-29 at 11:57 +0100, Krzysztof Mazur wrote:
> do we really need to wait here?
> Why don't just do something like that:
>
> tasklet_disable(&card->tlet);
> spin_lock(&card->tx_queue_lock);
> for each skb in queue
> SKB_CB(skb)->vcc = NULL;
> spin_
On Wed, Nov 28, 2012 at 10:18:35PM +, David Woodhouse wrote:
> On Wed, 2012-11-28 at 09:21 +, David Laight wrote:
> > Even when it might make sense to sleep in close until tx drains
> > there needs to be a finite timeout before it become abortive.
>
> You are, of course, right. We should n
On Wed, 2012-11-28 at 09:21 +, David Laight wrote:
> Even when it might make sense to sleep in close until tx drains
> there needs to be a finite timeout before it become abortive.
You are, of course, right. We should never wait for hardware for ever.
And just to serve me right, I seem to have
On Wed, Nov 28, 2012 at 08:44:41PM +, David Woodhouse wrote:
> On Wed, 2012-11-28 at 21:18 +0100, Krzysztof Mazur wrote:
> >
> > +void vcc_pop_any(struct atm_vcc *vcc, struct sk_buff *skb)
> > +{
> > + if (vcc->pop)
> > + vcc->pop(vcc, skb);
>
> if (vcc && vcc->pop) perhap
On Wed, 28 Nov 2012 21:18:37 +0100
Krzysztof Mazur wrote:
> On Tue, Nov 27, 2012 at 07:28:43PM +0100, Krzysztof Mazur wrote:
> > I think that we should add atm_pop() function that does that and fix all
> > drivers.
> >
>
> I'm sending a patch that implements that idea.
>
> Currently we need tw
On Wed, 2012-11-28 at 21:18 +0100, Krzysztof Mazur wrote:
>
> +void vcc_pop_any(struct atm_vcc *vcc, struct sk_buff *skb)
> +{
> + if (vcc->pop)
> + vcc->pop(vcc, skb);
if (vcc && vcc->pop) perhaps?
--
dwmw2
smime.p7s
Description: S/MIME cryptographic signature
On Tue, Nov 27, 2012 at 07:28:43PM +0100, Krzysztof Mazur wrote:
>
> While reviewing your br2684 patch I also found that some ATM drivers does
> not call ->pop() when ->send() fails, they should do:
>
> if (vcc->pop)
> vcc->pop(vcc, skb);
> else
> dev_kfree
On Wed, 28 Nov 2012 10:24:28 +
David Woodhouse wrote:
> On Wed, 2012-11-28 at 11:04 +0100, Krzysztof Mazur wrote:
> >
> > The ->close() routine can just abort any pending rx/tx and just wait
> > for completion of currently running rx/tx code. That shouldn't take
> > long.
>
> If it's been s
On Wed, 2012-11-28 at 11:04 +0100, Krzysztof Mazur wrote:
>
> The ->close() routine can just abort any pending rx/tx and just wait
> for completion of currently running rx/tx code. That shouldn't take
> long.
If it's been submitted to the hardware for DMA, it can't do that very
easily.
And if I
On Wed, Nov 28, 2012 at 09:21:37AM -, David Laight wrote:
> > On Tue, 27 Nov 2012 18:02:29 +
> > David Woodhouse wrote:
> >
> > > In solos-pci at least, the ops->close() function doesn't flush all
> > > pending skbs for this vcc before returning. So can be a tasklet
> > > somewhere which
> On Tue, 27 Nov 2012 18:02:29 +
> David Woodhouse wrote:
>
> > In solos-pci at least, the ops->close() function doesn't flush all
> > pending skbs for this vcc before returning. So can be a tasklet
> > somewhere which has loaded the address of the vcc->pop function from one
> > of them, and
On Tue, 27 Nov 2012 18:02:29 +
David Woodhouse wrote:
> In solos-pci at least, the ops->close() function doesn't flush all
> pending skbs for this vcc before returning. So can be a tasklet
> somewhere which has loaded the address of the vcc->pop function from one
> of them, and is going to ca
On Tue, Nov 27, 2012 at 06:02:29PM +, David Woodhouse wrote:
>
> I'm not running with that patch. This bug exists for br2684 even before
> it, and I think also for pppoatm.
>
Did you use your "atm: br2684: Fix excessive queue bloat" patch?
With that patch for pppoatm the dev->close()/pppoat
On Tue, Nov 27, 2012 at 06:02:29PM +, David Woodhouse wrote:
> On Tue, 2012-11-27 at 18:39 +0100, Krzysztof Mazur wrote:
> > Yes, I missed that one - it's even worse, I introduced that bug
> > in "[PATCH 1/7] atm: detach protocol before closing vcc". Before that
> > patch that scenario shouldn
On Tue, 2012-11-27 at 18:39 +0100, Krzysztof Mazur wrote:
> Yes, I missed that one - it's even worse, I introduced that bug
> in "[PATCH 1/7] atm: detach protocol before closing vcc". Before that
> patch that scenario shouldn't happen because vcc was closed before
> calling pppoatm_send(vcc, NULL)
On Tue, Nov 27, 2012 at 05:16:32PM +, David Woodhouse wrote:
> Krzysztof, you've fixed a bunch of races... but I think there's one
> still left.
>
> An ATM driver will often have code like this, which gets called from
> arbitrary contexts:
> if (vcc->pop)
> vcc->pop(vcc
Krzysztof, you've fixed a bunch of races... but I think there's one
still left.
An ATM driver will often have code like this, which gets called from
arbitrary contexts:
if (vcc->pop)
vcc->pop(vcc, skb);
Now, what happens if pppoatm_send(vcc, NULL) happens after the address
On Tue, Oct 30, 2012 at 09:39:22AM +, David Woodhouse wrote:
> On Mon, 2012-10-22 at 19:14 +0200, Krzysztof Mazur wrote:
> > The pppoatm gets a reference to atmvcc, but does not increment vcc
> > usage count. The vcc uses vcc->sk socket for reference counting,
> > so sock_hold() and sock_put()
On Mon, 2012-10-22 at 19:14 +0200, Krzysztof Mazur wrote:
> The pppoatm gets a reference to atmvcc, but does not increment vcc
> usage count. The vcc uses vcc->sk socket for reference counting,
> so sock_hold() and sock_put() should be used by pppoatm.
>
> Signed-off-by: Krzysztof Mazur
> Cc: Dav
42 matches
Mail list logo
