On 06/15/2018 05:29 AM, David Woodhouse wrote:
> On Fri, 2017-06-30 at 13:08 +0300, Elena Reshetova wrote:
>> diff --git a/include/linux/atmdev.h b/include/linux/atmdev.h
>> index c1da539..4d97a89 100644
>> --- a/include/linux/atmdev.h
>> +++ b/include/linux/atmdev.h
>> @@ -254,7 +254,7 @@ static inline void atm_return(struct atm_vcc *vcc,int
>> truesize)
>>
>> static inline int atm_may_send(struct atm_vcc *vcc,unsigned int size)
>> {
>> - return (size + atomic_read(&sk_atm(vcc)->sk_wmem_alloc)) <
>> + return (size + refcount_read(&sk_atm(vcc)->sk_wmem_alloc)) <
>> sk_atm(vcc)->sk_sndbuf;
>> }
>
>
> Hm, this (commit 14afee4b6092fd) may have broken PPPoATM. I did spend a
> while staring hard at my own commit 9d02daf754238 which introduced
> pppoatm_may_send(), but it's actually atm_may_send() which is never
> allowing packets to be pushed.
>
> Debugging (which is ongoing) has so far shown that we are accounting
> for a packet in pppoatm_send() which has skb->truesize==0x1c0, and
> later end up in pppoatm_pop()→atm_raw_pop() with skb->truesize==0x2c0.
>
> This was always harmless before, but now it's a refcount_t it appears
> to underflow and go into its "screw you" mode and never let any more
> packets get sent.
>
> I'm staring hard at the special case in pskb_expand_head() to *not*
> change skb->truesize under certain circumstances, and wondering if that
> (is, should be) covering the case of ATM skbs:
>
> /* It is not generally safe to change skb->truesize.
> * For the moment, we really care of rx path, or
> * when skb is orphaned (not attached to a socket).
> */
> if (!skb->sk || skb->destructor == sock_edemux)
> skb->truesize += size - osize;
>
> Failing that, maybe we should copy the accounted value of skb->truesize
> into the struct skb_atm_data in skb->cb at the time we add it to
> sk_wmem_alloc — and then subtract *that* value from sk_wmem_alloc in
> atm_raw_pop() instead of the then current value of skb->truesize.
>
> Suggestions?
>
Maybe ATM should make sure skb->sk is set ?
something like the following :
diff --git a/net/atm/pppoatm.c b/net/atm/pppoatm.c
index
21d9d341a6199255a017437954e4b688f1ba5bfd..4863d02939a26ce0a5816da86779336255d550bd
100644
--- a/net/atm/pppoatm.c
+++ b/net/atm/pppoatm.c
@@ -350,7 +350,7 @@ static int pppoatm_send(struct ppp_channel *chan, struct
sk_buff *skb)
return 1;
}
- refcount_add(skb->truesize, &sk_atm(ATM_SKB(skb)->vcc)->sk_wmem_alloc);
+ skb_set_owner_w(skb, &sk_atm(ATM_SKB(skb)->vcc));
ATM_SKB(skb)->atm_options = ATM_SKB(skb)->vcc->atm_options;
pr_debug("atm_skb(%p)->vcc(%p)->dev(%p)\n",
skb, ATM_SKB(skb)->vcc, ATM_SKB(skb)->vcc->dev);
Or something more sophisticated, but this might need more thinking
diff --git a/net/atm/pppoatm.c b/net/atm/pppoatm.c
index
21d9d341a6199255a017437954e4b688f1ba5bfd..80902a0b56a1d15d2851a07f067490d99136d214
100644
--- a/net/atm/pppoatm.c
+++ b/net/atm/pppoatm.c
@@ -350,7 +350,10 @@ static int pppoatm_send(struct ppp_channel *chan, struct
sk_buff *skb)
return 1;
}
- refcount_add(skb->truesize, &sk_atm(ATM_SKB(skb)->vcc)->sk_wmem_alloc);
+ if (!skb->sk)
+ skb_set_owner_w(skb, &sk_atm(ATM_SKB(skb)->vcc));
+- else
+ refcount_add(skb->truesize,
&sk_atm(ATM_SKB(skb)->vcc)->sk_wmem_alloc);
ATM_SKB(skb)->atm_options = ATM_SKB(skb)->vcc->atm_options;
pr_debug("atm_skb(%p)->vcc(%p)->dev(%p)\n",
skb, ATM_SKB(skb)->vcc, ATM_SKB(skb)->vcc->dev);
