* [PATCH v1 1/2] pptp: Use macro and sizeof instead of literal number
@ 2016-08-18 4:47 fgao
2016-08-19 21:31 ` David Miller
2016-08-19 23:14 ` Philp Prindeville
0 siblings, 2 replies; 3+ messages in thread
From: fgao @ 2016-08-18 4:47 UTC (permalink / raw)
To: xeb, philipp, netdev; +Cc: gfree.wind, Gao Feng
From: Gao Feng <fgao@ikuai8.com>
Use existing macros like PPP_ADDRESS, SC_COMP_PROT and sizeof fixed
variables instead of original literal number to enhance readbility.
BTW, the original pptp_rcv uses literal number "12" as the param
of pskb_may_pull. Actually the "12" is less than the size of struct
pptp_gre_header. Now use the sizeof(*header) fixes this issue.
Signed-off-by: Gao Feng <fgao@ikuai8.com>
---
v1: Initial patch
drivers/net/ppp/pptp.c | 23 ++++++++++++-----------
1 file changed, 12 insertions(+), 11 deletions(-)
diff --git a/drivers/net/ppp/pptp.c b/drivers/net/ppp/pptp.c
index 1951b10..48c3701 100644
--- a/drivers/net/ppp/pptp.c
+++ b/drivers/net/ppp/pptp.c
@@ -54,6 +54,8 @@ static struct proto pptp_sk_proto __read_mostly;
static const struct ppp_channel_ops pptp_chan_ops;
static const struct proto_ops pptp_ops;
+static const u8 fixed_ppphdr[2] = {PPP_ALLSTATIONS, PPP_UI};
+
static struct pppox_sock *lookup_chan(u16 call_id, __be32 s_addr)
{
struct pppox_sock *sock;
@@ -167,7 +169,7 @@ static int pptp_xmit(struct ppp_channel *chan, struct sk_buff *skb)
tdev = rt->dst.dev;
- max_headroom = LL_RESERVED_SPACE(tdev) + sizeof(*iph) + sizeof(*hdr) + 2;
+ max_headroom = LL_RESERVED_SPACE(tdev) + sizeof(*iph) + sizeof(*hdr) + sizeof(fixed_ppphdr);
if (skb_headroom(skb) < max_headroom || skb_cloned(skb) || skb_shared(skb)) {
struct sk_buff *new_skb = skb_realloc_headroom(skb, max_headroom);
@@ -190,9 +192,9 @@ static int pptp_xmit(struct ppp_channel *chan, struct sk_buff *skb)
/* Put in the address/control bytes if necessary */
if ((opt->ppp_flags & SC_COMP_AC) == 0 || islcp) {
- data = skb_push(skb, 2);
- data[0] = PPP_ALLSTATIONS;
- data[1] = PPP_UI;
+ data = skb_push(skb, sizeof(fixed_ppphdr));
+ data[0] = fixed_ppphdr[0];
+ data[1] = fixed_ppphdr[1];
}
len = skb->len;
@@ -219,8 +221,7 @@ static int pptp_xmit(struct ppp_channel *chan, struct sk_buff *skb)
}
hdr->payload_len = htons(len);
- /* Push down and install the IP header. */
-
+ /* Push down and install the IP header. */
skb_reset_transport_header(skb);
skb_push(skb, sizeof(*iph));
skb_reset_network_header(skb);
@@ -319,14 +320,14 @@ static int pptp_rcv_core(struct sock *sk, struct sk_buff *skb)
allow_packet:
skb_pull(skb, headersize);
- if (payload[0] == PPP_ALLSTATIONS && payload[1] == PPP_UI) {
+ if (PPP_ADDRESS(payload) == PPP_ALLSTATIONS && PPP_CONTROL(payload) == PPP_UI) {
/* chop off address/control */
if (skb->len < 3)
goto drop;
- skb_pull(skb, 2);
+ skb_pull(skb, sizeof(fixed_ppphdr));
}
- if ((*skb->data) & 1) {
+ if ((*skb->data) & SC_COMP_PROT) {
/* protocol is compressed */
skb_push(skb, 1)[0] = 0;
}
@@ -351,7 +352,7 @@ static int pptp_rcv(struct sk_buff *skb)
if (skb->pkt_type != PACKET_HOST)
goto drop;
- if (!pskb_may_pull(skb, 12))
+ if (!pskb_may_pull(skb, sizeof(*header)))
goto drop;
iph = ip_hdr(skb);
@@ -468,7 +469,7 @@ static int pptp_connect(struct socket *sock, struct sockaddr *uservaddr,
ip_rt_put(rt);
po->chan.mtu -= PPTP_HEADER_OVERHEAD;
- po->chan.hdrlen = 2 + sizeof(struct pptp_gre_header);
+ po->chan.hdrlen = sizeof(fixed_ppphdr) + sizeof(struct pptp_gre_header);
error = ppp_register_channel(&po->chan);
if (error) {
pr_err("PPTP: failed to register PPP channel (%d)\n", error);
--
1.9.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v1 1/2] pptp: Use macro and sizeof instead of literal number
2016-08-18 4:47 [PATCH v1 1/2] pptp: Use macro and sizeof instead of literal number fgao
@ 2016-08-19 21:31 ` David Miller
2016-08-19 23:14 ` Philp Prindeville
1 sibling, 0 replies; 3+ messages in thread
From: David Miller @ 2016-08-19 21:31 UTC (permalink / raw)
To: fgao; +Cc: xeb, philipp, netdev, gfree.wind
From: fgao@ikuai8.com
Date: Thu, 18 Aug 2016 12:47:34 +0800
> @@ -54,6 +54,8 @@ static struct proto pptp_sk_proto __read_mostly;
> static const struct ppp_channel_ops pptp_chan_ops;
> static const struct proto_ops pptp_ops;
>
> +static const u8 fixed_ppphdr[2] = {PPP_ALLSTATIONS, PPP_UI};
> +
This makes things much worse in my opinion, and at the cost
of two extra bytes in the data section as well.
I'm not applying this.
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v1 1/2] pptp: Use macro and sizeof instead of literal number
2016-08-18 4:47 [PATCH v1 1/2] pptp: Use macro and sizeof instead of literal number fgao
2016-08-19 21:31 ` David Miller
@ 2016-08-19 23:14 ` Philp Prindeville
1 sibling, 0 replies; 3+ messages in thread
From: Philp Prindeville @ 2016-08-19 23:14 UTC (permalink / raw)
To: fgao, xeb, netdev; +Cc: gfree.wind
Inline...
On 08/17/2016 10:47 PM, fgao@48lvckh6395k16k5.yundunddos.com wrote:
> From: Gao Feng<fgao@ikuai8.com>
>
> Use existing macros like PPP_ADDRESS, SC_COMP_PROT and sizeof fixed
> variables instead of original literal number to enhance readbility.
>
> BTW, the original pptp_rcv uses literal number "12" as the param
> of pskb_may_pull. Actually the "12" is less than the size of struct
> pptp_gre_header. Now use the sizeof(*header) fixes this issue.
>
> Signed-off-by: Gao Feng<fgao@ikuai8.com>
> ---
> v1: Initial patch
>
> drivers/net/ppp/pptp.c | 23 ++++++++++++-----------
> 1 file changed, 12 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/net/ppp/pptp.c b/drivers/net/ppp/pptp.c
> index 1951b10..48c3701 100644
> --- a/drivers/net/ppp/pptp.c
> +++ b/drivers/net/ppp/pptp.c
> @@ -54,6 +54,8 @@ static struct proto pptp_sk_proto __read_mostly;
> static const struct ppp_channel_ops pptp_chan_ops;
> static const struct proto_ops pptp_ops;
>
> +static const u8 fixed_ppphdr[2] = {PPP_ALLSTATIONS, PPP_UI};
> +
The PPP header is actually 4 bytes, you're omitting 2 bytes of
protocol. See PPP_HDRLEN. Agree with Miller that we don't need this
mock header here.
> static struct pppox_sock *lookup_chan(u16 call_id, __be32 s_addr)
> {
> struct pppox_sock *sock;
> @@ -167,7 +169,7 @@ static int pptp_xmit(struct ppp_channel *chan, struct sk_buff *skb)
>
> tdev = rt->dst.dev;
>
> - max_headroom = LL_RESERVED_SPACE(tdev) + sizeof(*iph) + sizeof(*hdr) + 2;
> + max_headroom = LL_RESERVED_SPACE(tdev) + sizeof(*iph) + sizeof(*hdr) + sizeof(fixed_ppphdr);
Where are the 2 bytes of PPP protocol field being accounted for?
>
> if (skb_headroom(skb) < max_headroom || skb_cloned(skb) || skb_shared(skb)) {
> struct sk_buff *new_skb = skb_realloc_headroom(skb, max_headroom);
> @@ -190,9 +192,9 @@ static int pptp_xmit(struct ppp_channel *chan, struct sk_buff *skb)
>
> /* Put in the address/control bytes if necessary */
> if ((opt->ppp_flags & SC_COMP_AC) == 0 || islcp) {
> - data = skb_push(skb, 2);
> - data[0] = PPP_ALLSTATIONS;
> - data[1] = PPP_UI;
> + data = skb_push(skb, sizeof(fixed_ppphdr));
> + data[0] = fixed_ppphdr[0];
> + data[1] = fixed_ppphdr[1];
I'd use:
PPP_ADDRESS(data) = PPP_ALLSTATIONS;
PPP_CONTROL(data) = PPP_UI;
here.
> }
>
> len = skb->len;
> @@ -219,8 +221,7 @@ static int pptp_xmit(struct ppp_channel *chan, struct sk_buff *skb)
> }
> hdr->payload_len = htons(len);
>
> - /* Push down and install the IP header. */
> -
> + /* Push down and install the IP header. */
> skb_reset_transport_header(skb);
> skb_push(skb, sizeof(*iph));
> skb_reset_network_header(skb);
> @@ -319,14 +320,14 @@ static int pptp_rcv_core(struct sock *sk, struct sk_buff *skb)
> allow_packet:
> skb_pull(skb, headersize);
>
> - if (payload[0] == PPP_ALLSTATIONS && payload[1] == PPP_UI) {
> + if (PPP_ADDRESS(payload) == PPP_ALLSTATIONS && PPP_CONTROL(payload) == PPP_UI) {
> /* chop off address/control */
> if (skb->len < 3)
> goto drop;
> - skb_pull(skb, 2);
> + skb_pull(skb, sizeof(fixed_ppphdr));
> }
>
> - if ((*skb->data) & 1) {
> + if ((*skb->data) & SC_COMP_PROT) {
Before the skb_pull(skb, 2) happens above, I would extract the protocol
via PPP_CONTROL(data).
> /* protocol is compressed */
> skb_push(skb, 1)[0] = 0;
> }
> @@ -351,7 +352,7 @@ static int pptp_rcv(struct sk_buff *skb)
> if (skb->pkt_type != PACKET_HOST)
> goto drop;
>
> - if (!pskb_may_pull(skb, 12))
> + if (!pskb_may_pull(skb, sizeof(*header)))
> goto drop;
>
> iph = ip_hdr(skb);
> @@ -468,7 +469,7 @@ static int pptp_connect(struct socket *sock, struct sockaddr *uservaddr,
> ip_rt_put(rt);
> po->chan.mtu -= PPTP_HEADER_OVERHEAD;
>
> - po->chan.hdrlen = 2 + sizeof(struct pptp_gre_header);
> + po->chan.hdrlen = sizeof(fixed_ppphdr) + sizeof(struct pptp_gre_header);
> error = ppp_register_channel(&po->chan);
> if (error) {
> pr_err("PPTP: failed to register PPP channel (%d)\n", error);
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-08-19 23:16 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-08-18 4:47 [PATCH v1 1/2] pptp: Use macro and sizeof instead of literal number fgao
2016-08-19 21:31 ` David Miller
2016-08-19 23:14 ` Philp Prindeville
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).