| Message ID | 1471708347-18063-1-git-send-email-fgao@ikuai8.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
From: fgao@ikuai8.com Date: Sat, 20 Aug 2016 23:52:27 +0800 > From: Gao Feng <fgao@ikuai8.com> > > Use PPP_ALLSTATIONS, PPP_UI, and SEND_SHUTDOWN instead of 0xff, > 0x03, and 2 separately. > > Signed-off-by: Gao Feng <fgao@ikuai8.com> Applied.
Inline On 08/20/2016 09:52 AM, fgao@48lvckh6395k16k5.yundunddos.com wrote: > From: Gao Feng <fgao@ikuai8.com> > > Use PPP_ALLSTATIONS, PPP_UI, and SEND_SHUTDOWN instead of 0xff, > 0x03, and 2 separately. > > Signed-off-by: Gao Feng <fgao@ikuai8.com> > --- > v3: Modify the subject; > v2: Only replace the literal number with macros according to Guillaume's advice > v1: Inital patch > > net/l2tp/l2tp_ppp.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c > index d9560aa..65e2fd6 100644 > --- a/net/l2tp/l2tp_ppp.c > +++ b/net/l2tp/l2tp_ppp.c > @@ -177,7 +177,7 @@ static int pppol2tp_recv_payload_hook(struct sk_buff *skb) > if (!pskb_may_pull(skb, 2)) > return 1; > > - if ((skb->data[0] == 0xff) && (skb->data[1] == 0x03)) > + if ((skb->data[0] == PPP_ALLSTATIONS) && (skb->data[1] == PPP_UI)) This should have used PPP_ADDRESS() and PPP_CONTROL() here. > skb_pull(skb, 2); This magic number should go away. > > return 0; > @@ -282,7 +282,7 @@ static void pppol2tp_session_sock_put(struct l2tp_session *session) > static int pppol2tp_sendmsg(struct socket *sock, struct msghdr *m, > size_t total_len) > { > - static const unsigned char ppph[2] = { 0xff, 0x03 }; > + static const unsigned char ppph[2] = {PPP_ALLSTATIONS, PPP_UI}; PPP has a 4-byte header. Where's the protocol value? > struct sock *sk = sock->sk; > struct sk_buff *skb; > int error; > @@ -369,7 +369,7 @@ error: > */ > static int pppol2tp_xmit(struct ppp_channel *chan, struct sk_buff *skb) > { > - static const u8 ppph[2] = { 0xff, 0x03 }; > + static const u8 ppph[2] = {PPP_ALLSTATIONS, PPP_UI}; Same as above. > struct sock *sk = (struct sock *) chan->private; > struct sock *sk_tun; > struct l2tp_session *session; > @@ -440,7 +440,7 @@ static void pppol2tp_session_close(struct l2tp_session *session) > BUG_ON(session->magic != L2TP_SESSION_MAGIC); > > if (sock) { > - inet_shutdown(sock, 2); > + inet_shutdown(sock, SEND_SHUTDOWN); > /* Don't let the session go away before our socket does */ > l2tp_session_inc_refcount(session); > }
From: Philp Prindeville <philipp@redfish-solutions.com> Date: Sun, 21 Aug 2016 16:36:52 -0600 > Inline Sorry, I applied this already, I'll revert it.
inline On Mon, Aug 22, 2016 at 6:36 AM, Philp Prindeville <philipp@redfish-solutions.com> wrote: > Inline > > > On 08/20/2016 09:52 AM, fgao@48lvckh6395k16k5.yundunddos.com wrote: >> >> From: Gao Feng <fgao@ikuai8.com> >> >> Use PPP_ALLSTATIONS, PPP_UI, and SEND_SHUTDOWN instead of 0xff, >> 0x03, and 2 separately. >> >> Signed-off-by: Gao Feng <fgao@ikuai8.com> >> --- >> v3: Modify the subject; >> v2: Only replace the literal number with macros according to Guillaume's >> advice >> v1: Inital patch >> >> net/l2tp/l2tp_ppp.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c >> index d9560aa..65e2fd6 100644 >> --- a/net/l2tp/l2tp_ppp.c >> +++ b/net/l2tp/l2tp_ppp.c >> @@ -177,7 +177,7 @@ static int pppol2tp_recv_payload_hook(struct sk_buff >> *skb) >> if (!pskb_may_pull(skb, 2)) >> return 1; >> - if ((skb->data[0] == 0xff) && (skb->data[1] == 0x03)) >> + if ((skb->data[0] == PPP_ALLSTATIONS) && (skb->data[1] == PPP_UI)) > > > This should have used PPP_ADDRESS() and PPP_CONTROL() here. In my initial patch, I replace them with PPP_ADDRESS() and PPP_CONTROL. But Guillaume thought it was not clear as before. So I revert it. > >> skb_pull(skb, 2); > > > This magic number should go away. Same as above. > >> return 0; >> @@ -282,7 +282,7 @@ static void pppol2tp_session_sock_put(struct >> l2tp_session *session) >> static int pppol2tp_sendmsg(struct socket *sock, struct msghdr *m, >> size_t total_len) >> { >> - static const unsigned char ppph[2] = { 0xff, 0x03 }; >> + static const unsigned char ppph[2] = {PPP_ALLSTATIONS, PPP_UI}; > > > PPP has a 4-byte header. Where's the protocol value? In the original code, I fail to find the code which is used to fill the protocol value. So I keep the two bytes header. And I thought the protocol value may be filled by the upper layer. > >> struct sock *sk = sock->sk; >> struct sk_buff *skb; >> int error; >> @@ -369,7 +369,7 @@ error: >> */ >> static int pppol2tp_xmit(struct ppp_channel *chan, struct sk_buff *skb) >> { >> - static const u8 ppph[2] = { 0xff, 0x03 }; >> + static const u8 ppph[2] = {PPP_ALLSTATIONS, PPP_UI}; > > > Same as above. > > > >> struct sock *sk = (struct sock *) chan->private; >> struct sock *sk_tun; >> struct l2tp_session *session; >> @@ -440,7 +440,7 @@ static void pppol2tp_session_close(struct l2tp_session >> *session) >> BUG_ON(session->magic != L2TP_SESSION_MAGIC); >> if (sock) { >> - inet_shutdown(sock, 2); >> + inet_shutdown(sock, SEND_SHUTDOWN); >> /* Don't let the session go away before our socket does */ >> l2tp_session_inc_refcount(session); >> } > >
On Sun, Aug 21, 2016 at 04:36:52PM -0600, Philp Prindeville wrote: > Inline > > > On 08/20/2016 09:52 AM, fgao@48lvckh6395k16k5.yundunddos.com wrote: > > From: Gao Feng <fgao@ikuai8.com> > > > > Use PPP_ALLSTATIONS, PPP_UI, and SEND_SHUTDOWN instead of 0xff, > > 0x03, and 2 separately. > > > > Signed-off-by: Gao Feng <fgao@ikuai8.com> > > --- > > v3: Modify the subject; > > v2: Only replace the literal number with macros according to Guillaume's advice > > v1: Inital patch > > > > net/l2tp/l2tp_ppp.c | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c > > index d9560aa..65e2fd6 100644 > > --- a/net/l2tp/l2tp_ppp.c > > +++ b/net/l2tp/l2tp_ppp.c > > @@ -177,7 +177,7 @@ static int pppol2tp_recv_payload_hook(struct sk_buff *skb) > > if (!pskb_may_pull(skb, 2)) > > return 1; > > - if ((skb->data[0] == 0xff) && (skb->data[1] == 0x03)) > > + if ((skb->data[0] == PPP_ALLSTATIONS) && (skb->data[1] == PPP_UI)) > > This should have used PPP_ADDRESS() and PPP_CONTROL() here. > Then please justify how would that make the code more readable. We're not trying to interpret a known valid PPP header here. > > skb_pull(skb, 2); > > This magic number should go away. > Again, this is *not* a magic number. We've explicitely accessed the first _two_ header bytes and want to skip them. pskb_may_pull(2), ->data[0], ->data[1] and skb_pull(2) all go together. There's even a nice comment telling you what is done and why: /* Skip PPP header, if present. In testing, Microsoft L2TP clients * don't send the PPP header (PPP header compression enabled), but * other clients can include the header. So we cope with both cases * here. The PPP header is always FF03 when using L2TP. * * Note that skb->data[] isn't dereferenced from a u16 ptr here since * the field may be unaligned. */ Apart from the unprecise "PPP header" term, which should be read as "address and control fields", things should be quite clear. > > @@ -282,7 +282,7 @@ static void pppol2tp_session_sock_put(struct l2tp_session *session) > > static int pppol2tp_sendmsg(struct socket *sock, struct msghdr *m, > > size_t total_len) > > { > > - static const unsigned char ppph[2] = { 0xff, 0x03 }; > > + static const unsigned char ppph[2] = {PPP_ALLSTATIONS, PPP_UI}; > > PPP has a 4-byte header. Where's the protocol value? > No, PPP header (whatever you include in it) is of variable length. And the protocol has already been set by the PPP layer anyway. We're in L2TP here.
On Mon, Aug 22, 2016 at 08:13:48AM +0800, Feng Gao wrote: > inline > > On Mon, Aug 22, 2016 at 6:36 AM, Philp Prindeville > <philipp@redfish-solutions.com> wrote: > > Inline > > > > > > On 08/20/2016 09:52 AM, fgao@48lvckh6395k16k5.yundunddos.com wrote: > >> > >> From: Gao Feng <fgao@ikuai8.com> > >> > >> Use PPP_ALLSTATIONS, PPP_UI, and SEND_SHUTDOWN instead of 0xff, > >> 0x03, and 2 separately. > >> > >> Signed-off-by: Gao Feng <fgao@ikuai8.com> > >> --- > >> v3: Modify the subject; > >> v2: Only replace the literal number with macros according to Guillaume's > >> advice > >> v1: Inital patch > >> > >> net/l2tp/l2tp_ppp.c | 8 ++++---- > >> 1 file changed, 4 insertions(+), 4 deletions(-) > >> > >> diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c > >> index d9560aa..65e2fd6 100644 > >> --- a/net/l2tp/l2tp_ppp.c > >> +++ b/net/l2tp/l2tp_ppp.c > >> @@ -177,7 +177,7 @@ static int pppol2tp_recv_payload_hook(struct sk_buff > >> *skb) > >> if (!pskb_may_pull(skb, 2)) > >> return 1; > >> - if ((skb->data[0] == 0xff) && (skb->data[1] == 0x03)) > >> + if ((skb->data[0] == PPP_ALLSTATIONS) && (skb->data[1] == PPP_UI)) > > > > > > This should have used PPP_ADDRESS() and PPP_CONTROL() here. > > In my initial patch, I replace them with PPP_ADDRESS() and PPP_CONTROL. > But Guillaume thought it was not clear as before. > So I revert it. > > > > >> skb_pull(skb, 2); > > > > > > This magic number should go away. > > Same as above. > > > > >> return 0; > >> @@ -282,7 +282,7 @@ static void pppol2tp_session_sock_put(struct > >> l2tp_session *session) > >> static int pppol2tp_sendmsg(struct socket *sock, struct msghdr *m, > >> size_t total_len) > >> { > >> - static const unsigned char ppph[2] = { 0xff, 0x03 }; > >> + static const unsigned char ppph[2] = {PPP_ALLSTATIONS, PPP_UI}; > > > > > > PPP has a 4-byte header. Where's the protocol value? > > In the original code, I fail to find the code which is used to fill > the protocol value. > So I keep the two bytes header. And I thought the protocol value may be filled > by the upper layer. > And you were right. This was a macro replacement patch anyway, so you didn't have to bring functional changes with it. And if the protocol field was really missing, the L2TP module would have never worked.
On Sat, Aug 20, 2016 at 11:52:27PM +0800, fgao@ikuai8.com wrote: > From: Gao Feng <fgao@ikuai8.com> > > Use PPP_ALLSTATIONS, PPP_UI, and SEND_SHUTDOWN instead of 0xff, > 0x03, and 2 separately. > > Signed-off-by: Gao Feng <fgao@ikuai8.com> > --- > v3: Modify the subject; > v2: Only replace the literal number with macros according to Guillaume's advice > v1: Inital patch > > net/l2tp/l2tp_ppp.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c > index d9560aa..65e2fd6 100644 > --- a/net/l2tp/l2tp_ppp.c > +++ b/net/l2tp/l2tp_ppp.c > @@ -177,7 +177,7 @@ static int pppol2tp_recv_payload_hook(struct sk_buff *skb) > if (!pskb_may_pull(skb, 2)) > return 1; > > - if ((skb->data[0] == 0xff) && (skb->data[1] == 0x03)) > + if ((skb->data[0] == PPP_ALLSTATIONS) && (skb->data[1] == PPP_UI)) > skb_pull(skb, 2); > > return 0; > @@ -282,7 +282,7 @@ static void pppol2tp_session_sock_put(struct l2tp_session *session) > static int pppol2tp_sendmsg(struct socket *sock, struct msghdr *m, > size_t total_len) > { > - static const unsigned char ppph[2] = { 0xff, 0x03 }; > + static const unsigned char ppph[2] = {PPP_ALLSTATIONS, PPP_UI}; > Minor nit: I'd prefer to keep the space after '{' and before '}'. I didn't want to bother you with this, but since it seems you'll have to repost... > struct sock *sk = sock->sk; > struct sk_buff *skb; > int error; > @@ -369,7 +369,7 @@ error: > */ > static int pppol2tp_xmit(struct ppp_channel *chan, struct sk_buff *skb) > { > - static const u8 ppph[2] = { 0xff, 0x03 }; > + static const u8 ppph[2] = {PPP_ALLSTATIONS, PPP_UI}; > Same here. BTW, I thought you also wanted to remove the static ppph variable from pppol2tp_xmit() / pppol2tp_sendmsg(), to directly assign skb->data[0/1] with PPP_ALLSTATIONS/PPP_UI.
inline On Mon, Aug 22, 2016 at 6:07 PM, Guillaume Nault <g.nault@alphalink.fr> wrote: > On Sat, Aug 20, 2016 at 11:52:27PM +0800, fgao@ikuai8.com wrote: >> From: Gao Feng <fgao@ikuai8.com> >> >> Use PPP_ALLSTATIONS, PPP_UI, and SEND_SHUTDOWN instead of 0xff, >> 0x03, and 2 separately. >> >> Signed-off-by: Gao Feng <fgao@ikuai8.com> >> --- >> v3: Modify the subject; >> v2: Only replace the literal number with macros according to Guillaume's advice >> v1: Inital patch >> >> net/l2tp/l2tp_ppp.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c >> index d9560aa..65e2fd6 100644 >> --- a/net/l2tp/l2tp_ppp.c >> +++ b/net/l2tp/l2tp_ppp.c >> @@ -177,7 +177,7 @@ static int pppol2tp_recv_payload_hook(struct sk_buff *skb) >> if (!pskb_may_pull(skb, 2)) >> return 1; >> >> - if ((skb->data[0] == 0xff) && (skb->data[1] == 0x03)) >> + if ((skb->data[0] == PPP_ALLSTATIONS) && (skb->data[1] == PPP_UI)) >> skb_pull(skb, 2); >> >> return 0; >> @@ -282,7 +282,7 @@ static void pppol2tp_session_sock_put(struct l2tp_session *session) >> static int pppol2tp_sendmsg(struct socket *sock, struct msghdr *m, >> size_t total_len) >> { >> - static const unsigned char ppph[2] = { 0xff, 0x03 }; >> + static const unsigned char ppph[2] = {PPP_ALLSTATIONS, PPP_UI}; >> > Minor nit: I'd prefer to keep the space after '{' and before '}'. > I didn't want to bother you with this, but since it seems you'll have > to repost... I don't know if it is the coding style of Linux kernel. > >> struct sock *sk = sock->sk; >> struct sk_buff *skb; >> int error; >> @@ -369,7 +369,7 @@ error: >> */ >> static int pppol2tp_xmit(struct ppp_channel *chan, struct sk_buff *skb) >> { >> - static const u8 ppph[2] = { 0xff, 0x03 }; >> + static const u8 ppph[2] = {PPP_ALLSTATIONS, PPP_UI}; >> > Same here. > > BTW, I thought you also wanted to remove the static ppph variable > from pppol2tp_xmit() / pppol2tp_sendmsg(), to directly assign > skb->data[0/1] with PPP_ALLSTATIONS/PPP_UI. If removed static ppph, there will be some codes which use literal "2" instead of sizeof ppph. Is it ok? Regards Feng
inline On Mon, Aug 22, 2016 at 5:48 PM, Guillaume Nault <g.nault@alphalink.fr> wrote: > On Sun, Aug 21, 2016 at 04:36:52PM -0600, Philp Prindeville wrote: >> Inline >> >> >> On 08/20/2016 09:52 AM, fgao@48lvckh6395k16k5.yundunddos.com wrote: >> > From: Gao Feng <fgao@ikuai8.com> >> > >> > Use PPP_ALLSTATIONS, PPP_UI, and SEND_SHUTDOWN instead of 0xff, >> > 0x03, and 2 separately. >> > >> > Signed-off-by: Gao Feng <fgao@ikuai8.com> >> > --- >> > v3: Modify the subject; >> > v2: Only replace the literal number with macros according to Guillaume's advice >> > v1: Inital patch >> > >> > net/l2tp/l2tp_ppp.c | 8 ++++---- >> > 1 file changed, 4 insertions(+), 4 deletions(-) >> > >> > diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c >> > index d9560aa..65e2fd6 100644 >> > --- a/net/l2tp/l2tp_ppp.c >> > +++ b/net/l2tp/l2tp_ppp.c >> > @@ -177,7 +177,7 @@ static int pppol2tp_recv_payload_hook(struct sk_buff *skb) >> > if (!pskb_may_pull(skb, 2)) >> > return 1; >> > - if ((skb->data[0] == 0xff) && (skb->data[1] == 0x03)) >> > + if ((skb->data[0] == PPP_ALLSTATIONS) && (skb->data[1] == PPP_UI)) >> >> This should have used PPP_ADDRESS() and PPP_CONTROL() here. >> > Then please justify how would that make the code more readable. > We're not trying to interpret a known valid PPP header here. > >> > skb_pull(skb, 2); >> >> This magic number should go away. >> > Again, this is *not* a magic number. We've explicitely accessed the > first _two_ header bytes and want to skip them. > pskb_may_pull(2), ->data[0], ->data[1] and skb_pull(2) all go together. > > There's even a nice comment telling you what is done and why: > /* Skip PPP header, if present. In testing, Microsoft L2TP clients > * don't send the PPP header (PPP header compression enabled), but > * other clients can include the header. So we cope with both cases > * here. The PPP header is always FF03 when using L2TP. > * > * Note that skb->data[] isn't dereferenced from a u16 ptr here since > * the field may be unaligned. > */ > Apart from the unprecise "PPP header" term, which should be read as > "address and control fields", things should be quite clear. If remove the static ppph, may be more clear. Because it will cause person think about the ppp header. Regards Feng > >> > @@ -282,7 +282,7 @@ static void pppol2tp_session_sock_put(struct l2tp_session *session) >> > static int pppol2tp_sendmsg(struct socket *sock, struct msghdr *m, >> > size_t total_len) >> > { >> > - static const unsigned char ppph[2] = { 0xff, 0x03 }; >> > + static const unsigned char ppph[2] = {PPP_ALLSTATIONS, PPP_UI}; >> >> PPP has a 4-byte header. Where's the protocol value? >> > No, PPP header (whatever you include in it) is of variable length. And > the protocol has already been set by the PPP layer anyway. > We're in L2TP here.
On Mon, Aug 22, 2016 at 06:22:42PM +0800, Feng Gao wrote: > inline > > On Mon, Aug 22, 2016 at 6:07 PM, Guillaume Nault <g.nault@alphalink.fr> wrote: > > On Sat, Aug 20, 2016 at 11:52:27PM +0800, fgao@ikuai8.com wrote: > >> From: Gao Feng <fgao@ikuai8.com> > >> @@ -282,7 +282,7 @@ static void pppol2tp_session_sock_put(struct l2tp_session *session) > >> static int pppol2tp_sendmsg(struct socket *sock, struct msghdr *m, > >> size_t total_len) > >> { > >> - static const unsigned char ppph[2] = { 0xff, 0x03 }; > >> + static const unsigned char ppph[2] = {PPP_ALLSTATIONS, PPP_UI}; > >> > > Minor nit: I'd prefer to keep the space after '{' and before '}'. > > I didn't want to bother you with this, but since it seems you'll have > > to repost... > > I don't know if it is the coding style of Linux kernel. > Both forms are used currently and I can't recall any explicit preference statement. So unless David has an opinion, you can just use the form you like the best. > > BTW, I thought you also wanted to remove the static ppph variable > > from pppol2tp_xmit() / pppol2tp_sendmsg(), to directly assign > > skb->data[0/1] with PPP_ALLSTATIONS/PPP_UI. > > If removed static ppph, there will be some codes which use literal "2" > instead of sizeof ppph. > Is it ok? > The literal "2" would be used in the sock_wmalloc() call only (or for assigning the headroom variable in the case of pppol2tp_xmit()). Given the number of data summed, I agree that having a plain "2" in the middle could look odd. You can either add a comment for each data summed (like in pppol2tp_xmit()), something like: sock_wmalloc(sk, NET_SKB_PAD + sizeof(struct iphdr) + /* IP header */ ... 2 + /* PPP Address and control field */ ...); Or use a simple macro like: /* Size of the PPP address and control fields */ #define PPP_ACF_LEN 2 Or event use macro and comment. That's up to you. You can even drop this change entirely if you prefer, I don't mind. I just raised this point because you said you'd remove ppph.
diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c index d9560aa..65e2fd6 100644 --- a/net/l2tp/l2tp_ppp.c +++ b/net/l2tp/l2tp_ppp.c @@ -177,7 +177,7 @@ static int pppol2tp_recv_payload_hook(struct sk_buff *skb) if (!pskb_may_pull(skb, 2)) return 1; - if ((skb->data[0] == 0xff) && (skb->data[1] == 0x03)) + if ((skb->data[0] == PPP_ALLSTATIONS) && (skb->data[1] == PPP_UI)) skb_pull(skb, 2); return 0; @@ -282,7 +282,7 @@ static void pppol2tp_session_sock_put(struct l2tp_session *session) static int pppol2tp_sendmsg(struct socket *sock, struct msghdr *m, size_t total_len) { - static const unsigned char ppph[2] = { 0xff, 0x03 }; + static const unsigned char ppph[2] = {PPP_ALLSTATIONS, PPP_UI}; struct sock *sk = sock->sk; struct sk_buff *skb; int error; @@ -369,7 +369,7 @@ error: */ static int pppol2tp_xmit(struct ppp_channel *chan, struct sk_buff *skb) { - static const u8 ppph[2] = { 0xff, 0x03 }; + static const u8 ppph[2] = {PPP_ALLSTATIONS, PPP_UI}; struct sock *sk = (struct sock *) chan->private; struct sock *sk_tun; struct l2tp_session *session; @@ -440,7 +440,7 @@ static void pppol2tp_session_close(struct l2tp_session *session) BUG_ON(session->magic != L2TP_SESSION_MAGIC); if (sock) { - inet_shutdown(sock, 2); + inet_shutdown(sock, SEND_SHUTDOWN); /* Don't let the session go away before our socket does */ l2tp_session_inc_refcount(session); }