[PATCH v1 1/1] pppoe: l2tp: the PPPOX_CONNECTED should be used with bit operation

From: Gao Feng <redacted>

There are some codes in pppoe and l2tp which use the PPPOX_CONNECTED
as the value including assignment and condition check.
They should keep consistent with other codes.

Signed-off-by: Gao Feng <redacted>
---
 v1: Initial Patch

 drivers/net/ppp/pppoe.c | 2 +-
 net/l2tp/l2tp_ppp.c     | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ppp/pppoe.c b/drivers/net/ppp/pppoe.c
index 4ddae81..684b773 100644
--- a/drivers/net/ppp/pppoe.c
+++ b/drivers/net/ppp/pppoe.c

@@ -697,7 +697,7 @@ static int pppoe_connect(struct socket *sock, struct sockaddr *uservaddr,
 			goto err_put;
 		}
 
-		sk->sk_state = PPPOX_CONNECTED;
+		sk->sk_state |= PPPOX_CONNECTED;
 	}
 
 	po->num = sp->sa_addr.pppoe.sid;

diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c
index d9560aa..3984385 100644
--- a/net/l2tp/l2tp_ppp.c
+++ b/net/l2tp/l2tp_ppp.c

@@ -774,7 +774,7 @@ static int pppol2tp_connect(struct socket *sock, struct sockaddr *uservaddr,
 out_no_ppp:
 	/* This is how we get the session context from the socket. */
 	sk->sk_user_data = session;
-	sk->sk_state = PPPOX_CONNECTED;
+	sk->sk_state |= PPPOX_CONNECTED;
 	l2tp_info(session, PPPOL2TP_MSG_CONTROL, "%s: created\n",
 		  session->name);
 

@@ -856,7 +856,7 @@ static int pppol2tp_getname(struct socket *sock, struct sockaddr *uaddr,
 	error = -ENOTCONN;
 	if (sk == NULL)
 		goto end;
-	if (sk->sk_state != PPPOX_CONNECTED)
+	if (!(sk->sk_state & PPPOX_CONNECTED))
 		goto end;
 
 	error = -EBADF;

-- 
1.9.1

Reviewed-by: Philip Prindeville <redacted>


On 08/17/2016 07:59 PM, fgao@48lvckh6395k16k5.yundunddos.com wrote:

From: Gao Feng<redacted>

There are some codes in pppoe and l2tp which use the PPPOX_CONNECTED
as the value including assignment and condition check.
They should keep consistent with other codes.

Signed-off-by: Gao Feng<redacted>

On Thu, Aug 18, 2016 at 09:59:03AM +0800, fgao@ikuai8.com wrote:

Using plain assignment makes it clear for the reader that other flags
are unset. I see no reason for changing this.

Same here.

Looks like it was a bug. This one is worth a separate patch.

inline.

On Fri, Aug 19, 2016 at 1:44 AM, Guillaume Nault [off-list ref] wrote:

On Thu, Aug 18, 2016 at 09:59:03AM +0800, fgao@ikuai8.com wrote:

quoted

From: Gao Feng <redacted>

There are some codes in pppoe and l2tp which use the PPPOX_CONNECTED
as the value including assignment and condition check.
They should keep consistent with other codes.

Signed-off-by: Gao Feng <redacted>
---
 v1: Initial Patch

 drivers/net/ppp/pppoe.c | 2 +-
 net/l2tp/l2tp_ppp.c     | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ppp/pppoe.c b/drivers/net/ppp/pppoe.c
index 4ddae81..684b773 100644
--- a/drivers/net/ppp/pppoe.c
+++ b/drivers/net/ppp/pppoe.c

@@ -697,7 +697,7 @@ static int pppoe_connect(struct socket *sock, struct sockaddr *uservaddr,
                      goto err_put;
              }

-             sk->sk_state = PPPOX_CONNECTED;
+             sk->sk_state |= PPPOX_CONNECTED;

Using plain assignment makes it clear for the reader that other flags
are unset. I see no reason for changing this.

I get you. So I don't modify the PPPOX_DEAD assignment.
But I am afraid if there is some case that the flag PPPOX_BOUND is set
before PPPOX_CONNECTED . Then the assignment of "PPPOX_CONNECTED" will
clear the PPPOX_BOUND flag.

quoted

diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c
index d9560aa..3984385 100644
--- a/net/l2tp/l2tp_ppp.c
+++ b/net/l2tp/l2tp_ppp.c

@@ -774,7 +774,7 @@ static int pppol2tp_connect(struct socket *sock, struct sockaddr *uservaddr,
 out_no_ppp:
      /* This is how we get the session context from the socket. */
      sk->sk_user_data = session;
-     sk->sk_state = PPPOX_CONNECTED;
+     sk->sk_state |= PPPOX_CONNECTED;

Same here.

quoted

@@ -856,7 +856,7 @@ static int pppol2tp_getname(struct socket *sock, struct sockaddr *uaddr,
      error = -ENOTCONN;
      if (sk == NULL)
              goto end;
-     if (sk->sk_state != PPPOX_CONNECTED)
+     if (!(sk->sk_state & PPPOX_CONNECTED))

Looks like it was a bug. This one is worth a separate patch.

Ok, I send another patch for this bug.

Regards
Feng

On Fri, Aug 19, 2016 at 06:58:58AM +0800, Feng Gao wrote:

inline.

On Fri, Aug 19, 2016 at 1:44 AM, Guillaume Nault [off-list ref] wrote:

quoted

On Thu, Aug 18, 2016 at 09:59:03AM +0800, fgao@ikuai8.com wrote:

quoted

From: Gao Feng <redacted>

There are some codes in pppoe and l2tp which use the PPPOX_CONNECTED
as the value including assignment and condition check.
They should keep consistent with other codes.

Signed-off-by: Gao Feng <redacted>
---
 v1: Initial Patch

 drivers/net/ppp/pppoe.c | 2 +-
 net/l2tp/l2tp_ppp.c     | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ppp/pppoe.c b/drivers/net/ppp/pppoe.c
index 4ddae81..684b773 100644
--- a/drivers/net/ppp/pppoe.c
+++ b/drivers/net/ppp/pppoe.c

@@ -697,7 +697,7 @@ static int pppoe_connect(struct socket *sock, struct sockaddr *uservaddr,
                      goto err_put;
              }

-             sk->sk_state = PPPOX_CONNECTED;
+             sk->sk_state |= PPPOX_CONNECTED;

Using plain assignment makes it clear for the reader that other flags
are unset. I see no reason for changing this.

I get you. So I don't modify the PPPOX_DEAD assignment.
But I am afraid if there is some case that the flag PPPOX_BOUND is set
before PPPOX_CONNECTED . Then the assignment of "PPPOX_CONNECTED" will
clear the PPPOX_BOUND flag.

PPPOX_BOUND shouldn't be set here. If the socket hasn't been connected
before, it can't have the BOUND flag (pppox_ioctl(PPPIOCGCHAN) would
fail). But if it was connected, then it'd have to go through the
'/* Delete the old binding */' part of pppoe_connect() first, thus
reseting sk_state to PPPOX_NONE.