Commit db7bf6d9 authored by Herbert Xu's avatar Herbert Xu Committed by David S. Miller

[PPP] pppoe: Fix data clobbering in __pppoe_xmit and return value

The function __pppoe_xmit modifies the skb data and therefore it needs
to copy and skb data if it's cloned.

In fact, it currently allocates a new skb so that it can return 0 in
case of error without freeing the original skb.  This is totally wrong
because returning zero is meant to indicate congestion whereupon pppoe
is supposed to wake up the upper layer once the congestion subsides.

This makes sense for ppp_async and ppp_sync but is out-of-place for
pppoe.  This patch makes it always return 1 and free the skb.
Signed-off-by: default avatarHerbert Xu <herbert@gondor.apana.org.au>
Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
parent 31bac444
...@@ -850,9 +850,7 @@ static int __pppoe_xmit(struct sock *sk, struct sk_buff *skb) ...@@ -850,9 +850,7 @@ static int __pppoe_xmit(struct sock *sk, struct sk_buff *skb)
struct net_device *dev = po->pppoe_dev; struct net_device *dev = po->pppoe_dev;
struct pppoe_hdr hdr; struct pppoe_hdr hdr;
struct pppoe_hdr *ph; struct pppoe_hdr *ph;
int headroom = skb_headroom(skb);
int data_len = skb->len; int data_len = skb->len;
struct sk_buff *skb2;
if (sock_flag(sk, SOCK_DEAD) || !(sk->sk_state & PPPOX_CONNECTED)) if (sock_flag(sk, SOCK_DEAD) || !(sk->sk_state & PPPOX_CONNECTED))
goto abort; goto abort;
...@@ -866,53 +864,31 @@ static int __pppoe_xmit(struct sock *sk, struct sk_buff *skb) ...@@ -866,53 +864,31 @@ static int __pppoe_xmit(struct sock *sk, struct sk_buff *skb)
if (!dev) if (!dev)
goto abort; goto abort;
/* Copy the skb if there is no space for the header. */ /* Copy the data if there is no space for the header or if it's
if (headroom < (sizeof(struct pppoe_hdr) + dev->hard_header_len)) { * read-only.
skb2 = dev_alloc_skb(32+skb->len +
sizeof(struct pppoe_hdr) +
dev->hard_header_len);
if (skb2 == NULL)
goto abort;
skb_reserve(skb2, dev->hard_header_len + sizeof(struct pppoe_hdr));
skb_copy_from_linear_data(skb, skb_put(skb2, skb->len),
skb->len);
} else {
/* Make a clone so as to not disturb the original skb,
* give dev_queue_xmit something it can free.
*/ */
skb2 = skb_clone(skb, GFP_ATOMIC); if (skb_cow(skb, sizeof(*ph) + dev->hard_header_len))
if (skb2 == NULL)
goto abort; goto abort;
}
ph = (struct pppoe_hdr *) skb_push(skb2, sizeof(struct pppoe_hdr)); ph = (struct pppoe_hdr *) skb_push(skb, sizeof(struct pppoe_hdr));
memcpy(ph, &hdr, sizeof(struct pppoe_hdr)); memcpy(ph, &hdr, sizeof(struct pppoe_hdr));
skb2->protocol = __constant_htons(ETH_P_PPP_SES); skb->protocol = __constant_htons(ETH_P_PPP_SES);
skb_reset_network_header(skb2); skb_reset_network_header(skb);
skb2->dev = dev; skb->dev = dev;
dev->hard_header(skb2, dev, ETH_P_PPP_SES, dev->hard_header(skb, dev, ETH_P_PPP_SES,
po->pppoe_pa.remote, NULL, data_len); po->pppoe_pa.remote, NULL, data_len);
/* We're transmitting skb2, and assuming that dev_queue_xmit if (dev_queue_xmit(skb) < 0)
* will free it. The generic ppp layer however, is expecting
* that we give back 'skb' (not 'skb2') in case of failure,
* but free it in case of success.
*/
if (dev_queue_xmit(skb2) < 0)
goto abort; goto abort;
kfree_skb(skb);
return 1; return 1;
abort: abort:
return 0; kfree_skb(skb);
return 1;
} }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment