[PATCH] Advertise PPPoE MTU / avoid memory leak.

[PATCH] Advertise PPPoE MTU / avoid memory leak.

[mostrows]

PPPoE must advertise the underlying device's MTU via the ppp channel
descriptor structure, as multilink functionality depends on it.

__pppoe_xmit must free any skb it allocates if there is an error
submitting the skb downstream.

Signed-off-by: Michal Ostrowski <mostrows@earthlink.net>
---
 drivers/net/pppoe.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/drivers/net/pppoe.c b/drivers/net/pppoe.c
index 475dc93..b4dc516 100644
--- a/drivers/net/pppoe.c
+++ b/drivers/net/pppoe.c
@@ -600,6 +600,7 @@ static int pppoe_connect(struct socket *
 		po->chan.hdrlen = (sizeof(struct pppoe_hdr) +
 				   dev->hard_header_len);
 
+		po->chan.mtu = dev->mtu - sizeof(struct pppoe_hdr);
 		po->chan.private = sk;
 		po->chan.ops = &pppoe_chan_ops;
 
@@ -831,7 +832,7 @@ static int __pppoe_xmit(struct sock *sk,
 	struct pppoe_hdr *ph;
 	int headroom = skb_headroom(skb);
 	int data_len = skb->len;
-	struct sk_buff *skb2;
+	struct sk_buff *skb2 = NULL;
 
 	if (sock_flag(sk, SOCK_DEAD) || !(sk->sk_state & PPPOX_CONNECTED))
 		goto abort;
@@ -887,6 +888,8 @@ static int __pppoe_xmit(struct sock *sk,
 	return 1;
 
 abort:
+	if (skb2)
+		kfree_skb(skb2);
 	return 0;
 }
 
-- 
1.4.1.1

Re: [PATCH] Advertise PPPoE MTU / avoid memory leak.

[David Miller]

From: mostrows@earthlink.net
Date: Sat, 23 Sep 2006 12:30:23 -0500

> __pppoe_xmit must free any skb it allocates if there is an error
> submitting the skb downstream.

This isn't right, dev_queue_xmit() can return -ENETDOWN and still
free the SKB, so your change will cause the SKB to be freed up
twice in that case, from dev_queue_xmit():

	rc = -ENETDOWN;
	rcu_read_unlock_bh();

out_kfree_skb:
	kfree_skb(skb);
	return rc;

dev_queue_xmit() is basically expected to consume the packet,
error or not.

What case of calling dev_queue_xmit() did you discover that did not
kfree the SKB on error?  We should fix that.  On a quick scan on the
entire dev_queue_xmit() implmentation, I cannot find such a case.

Re: [PATCH] Advertise PPPoE MTU / avoid memory leak.

[Michal Ostrowski]

On Sat, 2006-09-23 at 14:56 -0700, David Miller wrote:
> From: mostrows@earthlink.net
> Date: Sat, 23 Sep 2006 12:30:23 -0500
> 
> > __pppoe_xmit must free any skb it allocates if there is an error
> > submitting the skb downstream.
> 
> This isn't right, dev_queue_xmit() can return -ENETDOWN and still
> free the SKB, so your change will cause the SKB to be freed up
> twice in that case, from dev_queue_xmit():
> 
> 	rc = -ENETDOWN;
> 	rcu_read_unlock_bh();
> 
> out_kfree_skb:
> 	kfree_skb(skb);
> 	return rc;
> 
> dev_queue_xmit() is basically expected to consume the packet,
> error or not.
> 
> What case of calling dev_queue_xmit() did you discover that did not
> kfree the SKB on error?  We should fix that.  On a quick scan on the
> entire dev_queue_xmit() implmentation, I cannot find such a case.
> 

I think the call path via dev->hard_start_xmit, if it fails, may result
in an skb not being freed.  This appears to be the case with the e100.c
driver.  The qdisc_restart path to dev->hard_start_xmit also appears
susceptible to this.  It appears that not all devices agree as to who
should clean-up an skb on error.

-- 
Michal Ostrowski <mostrows@earthlink.net>

Re: [PATCH] Advertise PPPoE MTU / avoid memory leak.

[David Miller]

From: Michal Ostrowski <mostrows@earthlink.net>
Date: Sun, 24 Sep 2006 07:29:25 -0500

> I think the call path via dev->hard_start_xmit, if it fails, may result
> in an skb not being freed.  This appears to be the case with the e100.c
> driver.  The qdisc_restart path to dev->hard_start_xmit also appears
> susceptible to this.  It appears that not all devices agree as to who
> should clean-up an skb on error.

There is a well defined policy about who frees the SKB or has
ownership of it based upon dev->hard_start_xmit() return values.

Any driver deviating from this set of rules should simply be
audited and fixed, as needed.

But, no matter, your change is buggy and we can't apply your
patch (even if it does fix a leak in some legitimate case)
because it introduces an obvious double-free bug.

Re: [PATCH] Advertise PPPoE MTU / avoid memory leak.

[Michal Ostrowski]

On Sun, 2006-09-24 at 18:41 -0700, David Miller wrote:
> From: Michal Ostrowski <mostrows@earthlink.net>
> Date: Sun, 24 Sep 2006 07:29:25 -0500
> 
> > I think the call path via dev->hard_start_xmit, if it fails, may result
> > in an skb not being freed.  This appears to be the case with the e100.c
> > driver.  The qdisc_restart path to dev->hard_start_xmit also appears
> > susceptible to this.  It appears that not all devices agree as to who
> > should clean-up an skb on error.
> 
> There is a well defined policy about who frees the SKB or has
> ownership of it based upon dev->hard_start_xmit() return values.
> 
> Any driver deviating from this set of rules should simply be
> audited and fixed, as needed.
> 
> But, no matter, your change is buggy and we can't apply your
> patch (even if it does fix a leak in some legitimate case)
> because it introduces an obvious double-free bug.
> 

Yup.  I'll resubmit a fixed one.


-- 
Michal Ostrowski <mostrows@earthlink.net>

Re: [PATCH] Advertise PPPoE MTU / avoid memory leak.

[Pekka Savola]

Speaking of PPPoE and MTU, does Linux support recently-published RFC 
4638:

   Accommodating a Maximum Transit Unit/Maximum Receive Unit (MTU/MRU)
                        Greater Than 1492 in the
              Point-to-Point Protocol over Ethernet (PPPoE)

On Sat, 23 Sep 2006, mostrows@earthlink.net wrote:
> PPPoE must advertise the underlying device's MTU via the ppp channel
> descriptor structure, as multilink functionality depends on it.
>
> __pppoe_xmit must free any skb it allocates if there is an error
> submitting the skb downstream.
>
> Signed-off-by: Michal Ostrowski <mostrows@earthlink.net>
> ---
> drivers/net/pppoe.c |    5 ++++-
> 1 files changed, 4 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/net/pppoe.c b/drivers/net/pppoe.c
> index 475dc93..b4dc516 100644
> --- a/drivers/net/pppoe.c
> +++ b/drivers/net/pppoe.c
> @@ -600,6 +600,7 @@ static int pppoe_connect(struct socket *
> 		po->chan.hdrlen = (sizeof(struct pppoe_hdr) +
> 				   dev->hard_header_len);
>
> +		po->chan.mtu = dev->mtu - sizeof(struct pppoe_hdr);
> 		po->chan.private = sk;
> 		po->chan.ops = &pppoe_chan_ops;
>
> @@ -831,7 +832,7 @@ static int __pppoe_xmit(struct sock *sk,
> 	struct pppoe_hdr *ph;
> 	int headroom = skb_headroom(skb);
> 	int data_len = skb->len;
> -	struct sk_buff *skb2;
> +	struct sk_buff *skb2 = NULL;
>
> 	if (sock_flag(sk, SOCK_DEAD) || !(sk->sk_state & PPPOX_CONNECTED))
> 		goto abort;
> @@ -887,6 +888,8 @@ static int __pppoe_xmit(struct sock *sk,
> 	return 1;
>
> abort:
> +	if (skb2)
> +		kfree_skb(skb2);
> 	return 0;
> }
>
>

-- 
Pekka Savola                 "You each name yourselves king, yet the
Netcore Oy                    kingdom bleeds."
Systems. Networks. Security. -- George R.R. Martin: A Clash of Kings

Re: [PATCH] Advertise PPPoE MTU / avoid memory leak.

[Michal Ostrowski]

This is not currently supported.

Looking at the RFC, this is an issue that must be solved in the pppoe
plugin that performs the PPPoE negotiation/discovery.  The kernel code
that runs the regular session traffic should be able to adjust (with
minimal or no changes) as it does not depend on a hardcoded MTU.



-- 
Michal Ostrowski <mostrows@earthlink.net>

On Tue, 2006-09-26 at 09:32 +0300, Pekka Savola wrote:
> Speaking of PPPoE and MTU, does Linux support recently-published RFC 
> 4638:
> 
>    Accommodating a Maximum Transit Unit/Maximum Receive Unit (MTU/MRU)
>                         Greater Than 1492 in the
>               Point-to-Point Protocol over Ethernet (PPPoE)
> 
> On Sat, 23 Sep 2006, mostrows@earthlink.net wrote:
> > PPPoE must advertise the underlying device's MTU via the ppp channel
> > descriptor structure, as multilink functionality depends on it.
> >
> > __pppoe_xmit must free any skb it allocates if there is an error
> > submitting the skb downstream.
> >
> > Signed-off-by: Michal Ostrowski <mostrows@earthlink.net>
> > ---
> > drivers/net/pppoe.c |    5 ++++-
> > 1 files changed, 4 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/net/pppoe.c b/drivers/net/pppoe.c
> > index 475dc93..b4dc516 100644
> > --- a/drivers/net/pppoe.c
> > +++ b/drivers/net/pppoe.c
> > @@ -600,6 +600,7 @@ static int pppoe_connect(struct socket *
> > 		po->chan.hdrlen = (sizeof(struct pppoe_hdr) +
> > 				   dev->hard_header_len);
> >
> > +		po->chan.mtu = dev->mtu - sizeof(struct pppoe_hdr);
> > 		po->chan.private = sk;
> > 		po->chan.ops = &pppoe_chan_ops;
> >
> > @@ -831,7 +832,7 @@ static int __pppoe_xmit(struct sock *sk,
> > 	struct pppoe_hdr *ph;
> > 	int headroom = skb_headroom(skb);
> > 	int data_len = skb->len;
> > -	struct sk_buff *skb2;
> > +	struct sk_buff *skb2 = NULL;
> >
> > 	if (sock_flag(sk, SOCK_DEAD) || !(sk->sk_state & PPPOX_CONNECTED))
> > 		goto abort;
> > @@ -887,6 +888,8 @@ static int __pppoe_xmit(struct sock *sk,
> > 	return 1;
> >
> > abort:
> > +	if (skb2)
> > +		kfree_skb(skb2);
> > 	return 0;
> > }
> >
> >
>