Re: [PATCH] Advertise PPPoE MTU / avoid memory leak.
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 [EMAIL PROTECTED] 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, [EMAIL PROTECTED] 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 [EMAIL PROTECTED] --- 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; } - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Advertise PPPoE MTU / avoid memory leak.
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, [EMAIL PROTECTED] 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 [EMAIL PROTECTED] --- 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 Oykingdom bleeds. Systems. Networks. Security. -- George R.R. Martin: A Clash of Kings - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Advertise PPPoE MTU / avoid memory leak.
On Sun, 2006-09-24 at 18:41 -0700, David Miller wrote: From: Michal Ostrowski [EMAIL PROTECTED] 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 [EMAIL PROTECTED] - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Advertise PPPoE MTU / avoid memory leak.
On Sat, 2006-09-23 at 14:56 -0700, David Miller wrote: From: [EMAIL PROTECTED] 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 [EMAIL PROTECTED] - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Advertise PPPoE MTU / avoid memory leak.
From: Michal Ostrowski [EMAIL PROTECTED] 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. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] Advertise PPPoE MTU / avoid memory leak.
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 [EMAIL PROTECTED] --- 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 - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Advertise PPPoE MTU / avoid memory leak.
From: [EMAIL PROTECTED] 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. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
