[Patch net] llc: properly handle dev_queue_xmit() return value

[Patch net] llc: properly handle dev_queue_xmit() return value

[Cong Wang]

llc_conn_send_pdu() pushes the skb into write queue and
calls llc_conn_send_pdus() to flush them out. However, the
status of dev_queue_xmit() is not returned to caller,
in this case, llc_conn_state_process().

llc_conn_state_process() needs hold the skb no matter
success or failure, because it still uses it after that,
therefore we should hold skb before dev_queue_xmit() when
that skb is the one being processed by llc_conn_state_process().

For other callers, they can just pass NULL and ignore
the return value as they are.

Reported-by: Noam Rathaus <noamr@beyondsecurity.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 include/net/llc_conn.h |  2 +-
 net/llc/llc_c_ac.c     | 15 +++++++++------
 net/llc/llc_conn.c     | 32 +++++++++++++++++++++++---------
 3 files changed, 33 insertions(+), 16 deletions(-)

diff --git a/include/net/llc_conn.h b/include/net/llc_conn.h
index fe994d2e5286..5c40f118c0fa 100644
--- a/include/net/llc_conn.h
+++ b/include/net/llc_conn.h
@@ -103,7 +103,7 @@ void llc_sk_reset(struct sock *sk);
 
 /* Access to a connection */
 int llc_conn_state_process(struct sock *sk, struct sk_buff *skb);
-void llc_conn_send_pdu(struct sock *sk, struct sk_buff *skb);
+int llc_conn_send_pdu(struct sock *sk, struct sk_buff *skb);
 void llc_conn_rtn_pdu(struct sock *sk, struct sk_buff *skb);
 void llc_conn_resend_i_pdu_as_cmd(struct sock *sk, u8 nr, u8 first_p_bit);
 void llc_conn_resend_i_pdu_as_rsp(struct sock *sk, u8 nr, u8 first_f_bit);
diff --git a/net/llc/llc_c_ac.c b/net/llc/llc_c_ac.c
index f59648018060..163121192aca 100644
--- a/net/llc/llc_c_ac.c
+++ b/net/llc/llc_c_ac.c
@@ -389,7 +389,7 @@ static int llc_conn_ac_send_i_cmd_p_set_0(struct sock *sk, struct sk_buff *skb)
 	llc_pdu_init_as_i_cmd(skb, 0, llc->vS, llc->vR);
 	rc = llc_mac_hdr_init(skb, llc->dev->dev_addr, llc->daddr.mac);
 	if (likely(!rc)) {
-		llc_conn_send_pdu(sk, skb);
+		rc = llc_conn_send_pdu(sk, skb);
 		llc_conn_ac_inc_vs_by_1(sk, skb);
 	}
 	return rc;
@@ -916,7 +916,7 @@ static int llc_conn_ac_send_i_rsp_f_set_ackpf(struct sock *sk,
 	llc_pdu_init_as_i_cmd(skb, llc->ack_pf, llc->vS, llc->vR);
 	rc = llc_mac_hdr_init(skb, llc->dev->dev_addr, llc->daddr.mac);
 	if (likely(!rc)) {
-		llc_conn_send_pdu(sk, skb);
+		rc = llc_conn_send_pdu(sk, skb);
 		llc_conn_ac_inc_vs_by_1(sk, skb);
 	}
 	return rc;
@@ -935,14 +935,17 @@ static int llc_conn_ac_send_i_rsp_f_set_ackpf(struct sock *sk,
 int llc_conn_ac_send_i_as_ack(struct sock *sk, struct sk_buff *skb)
 {
 	struct llc_sock *llc = llc_sk(sk);
+	int ret;
 
 	if (llc->ack_must_be_send) {
-		llc_conn_ac_send_i_rsp_f_set_ackpf(sk, skb);
+		ret = llc_conn_ac_send_i_rsp_f_set_ackpf(sk, skb);
 		llc->ack_must_be_send = 0 ;
 		llc->ack_pf = 0;
-	} else
-		llc_conn_ac_send_i_cmd_p_set_0(sk, skb);
-	return 0;
+	} else {
+		ret = llc_conn_ac_send_i_cmd_p_set_0(sk, skb);
+	}
+
+	return ret;
 }
 
 /**
diff --git a/net/llc/llc_conn.c b/net/llc/llc_conn.c
index 9177dbb16dce..110e32bcb399 100644
--- a/net/llc/llc_conn.c
+++ b/net/llc/llc_conn.c
@@ -30,7 +30,7 @@
 #endif
 
 static int llc_find_offset(int state, int ev_type);
-static void llc_conn_send_pdus(struct sock *sk);
+static int llc_conn_send_pdus(struct sock *sk, struct sk_buff *skb);
 static int llc_conn_service(struct sock *sk, struct sk_buff *skb);
 static int llc_exec_conn_trans_actions(struct sock *sk,
 				       struct llc_conn_state_trans *trans,
@@ -193,11 +193,11 @@ int llc_conn_state_process(struct sock *sk, struct sk_buff *skb)
 	return rc;
 }
 
-void llc_conn_send_pdu(struct sock *sk, struct sk_buff *skb)
+int llc_conn_send_pdu(struct sock *sk, struct sk_buff *skb)
 {
 	/* queue PDU to send to MAC layer */
 	skb_queue_tail(&sk->sk_write_queue, skb);
-	llc_conn_send_pdus(sk);
+	return llc_conn_send_pdus(sk, skb);
 }
 
 /**
@@ -255,7 +255,7 @@ void llc_conn_resend_i_pdu_as_cmd(struct sock *sk, u8 nr, u8 first_p_bit)
 	if (howmany_resend > 0)
 		llc->vS = (llc->vS + 1) % LLC_2_SEQ_NBR_MODULO;
 	/* any PDUs to re-send are queued up; start sending to MAC */
-	llc_conn_send_pdus(sk);
+	llc_conn_send_pdus(sk, NULL);
 out:;
 }
 
@@ -296,7 +296,7 @@ void llc_conn_resend_i_pdu_as_rsp(struct sock *sk, u8 nr, u8 first_f_bit)
 	if (howmany_resend > 0)
 		llc->vS = (llc->vS + 1) % LLC_2_SEQ_NBR_MODULO;
 	/* any PDUs to re-send are queued up; start sending to MAC */
-	llc_conn_send_pdus(sk);
+	llc_conn_send_pdus(sk, NULL);
 out:;
 }
 
@@ -340,12 +340,16 @@ int llc_conn_remove_acked_pdus(struct sock *sk, u8 nr, u16 *how_many_unacked)
 /**
  *	llc_conn_send_pdus - Sends queued PDUs
  *	@sk: active connection
+ *	@hold_skb: the skb held by caller, or NULL if does not care
  *
- *	Sends queued pdus to MAC layer for transmission.
+ *	Sends queued pdus to MAC layer for transmission. When @hold_skb is
+ *	NULL, always return 0. Otherwise, return 0 if @hold_skb is sent
+ *	successfully, or 1 for failure.
  */
-static void llc_conn_send_pdus(struct sock *sk)
+static int llc_conn_send_pdus(struct sock *sk, struct sk_buff *hold_skb)
 {
 	struct sk_buff *skb;
+	int ret = 0;
 
 	while ((skb = skb_dequeue(&sk->sk_write_queue)) != NULL) {
 		struct llc_pdu_sn *pdu = llc_pdu_sn_hdr(skb);
@@ -357,10 +361,20 @@ static void llc_conn_send_pdus(struct sock *sk)
 			skb_queue_tail(&llc_sk(sk)->pdu_unack_q, skb);
 			if (!skb2)
 				break;
-			skb = skb2;
+			dev_queue_xmit(skb2);
+		} else {
+			bool is_target = skb == hold_skb;
+			int rc;
+
+			if (is_target)
+				skb_get(skb);
+			rc = dev_queue_xmit(skb);
+			if (is_target)
+				ret = rc;
 		}
-		dev_queue_xmit(skb);
 	}
+
+	return ret;
 }
 
 /**
-- 
2.13.0

Re: [Patch net] llc: properly handle dev_queue_xmit() return value

[Noam Rathaus]

Hi,

I am not sure what is the next step from this?

Does it mean that a patch is out in the kernel's GIT/Beta version?

Or is this just a proposal?

On Tue, Mar 27, 2018 at 1:08 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> llc_conn_send_pdu() pushes the skb into write queue and
> calls llc_conn_send_pdus() to flush them out. However, the
> status of dev_queue_xmit() is not returned to caller,
> in this case, llc_conn_state_process().
>
> llc_conn_state_process() needs hold the skb no matter
> success or failure, because it still uses it after that,
> therefore we should hold skb before dev_queue_xmit() when
> that skb is the one being processed by llc_conn_state_process().
>
> For other callers, they can just pass NULL and ignore
> the return value as they are.
>
> Reported-by: Noam Rathaus <noamr@beyondsecurity.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> ---
>  include/net/llc_conn.h |  2 +-
>  net/llc/llc_c_ac.c     | 15 +++++++++------
>  net/llc/llc_conn.c     | 32 +++++++++++++++++++++++---------
>  3 files changed, 33 insertions(+), 16 deletions(-)
>
> diff --git a/include/net/llc_conn.h b/include/net/llc_conn.h
> index fe994d2e5286..5c40f118c0fa 100644
> --- a/include/net/llc_conn.h
> +++ b/include/net/llc_conn.h
> @@ -103,7 +103,7 @@ void llc_sk_reset(struct sock *sk);
>
>  /* Access to a connection */
>  int llc_conn_state_process(struct sock *sk, struct sk_buff *skb);
> -void llc_conn_send_pdu(struct sock *sk, struct sk_buff *skb);
> +int llc_conn_send_pdu(struct sock *sk, struct sk_buff *skb);
>  void llc_conn_rtn_pdu(struct sock *sk, struct sk_buff *skb);
>  void llc_conn_resend_i_pdu_as_cmd(struct sock *sk, u8 nr, u8 first_p_bit);
>  void llc_conn_resend_i_pdu_as_rsp(struct sock *sk, u8 nr, u8 first_f_bit);
> diff --git a/net/llc/llc_c_ac.c b/net/llc/llc_c_ac.c
> index f59648018060..163121192aca 100644
> --- a/net/llc/llc_c_ac.c
> +++ b/net/llc/llc_c_ac.c
> @@ -389,7 +389,7 @@ static int llc_conn_ac_send_i_cmd_p_set_0(struct sock *sk, struct sk_buff *skb)
>         llc_pdu_init_as_i_cmd(skb, 0, llc->vS, llc->vR);
>         rc = llc_mac_hdr_init(skb, llc->dev->dev_addr, llc->daddr.mac);
>         if (likely(!rc)) {
> -               llc_conn_send_pdu(sk, skb);
> +               rc = llc_conn_send_pdu(sk, skb);
>                 llc_conn_ac_inc_vs_by_1(sk, skb);
>         }
>         return rc;
> @@ -916,7 +916,7 @@ static int llc_conn_ac_send_i_rsp_f_set_ackpf(struct sock *sk,
>         llc_pdu_init_as_i_cmd(skb, llc->ack_pf, llc->vS, llc->vR);
>         rc = llc_mac_hdr_init(skb, llc->dev->dev_addr, llc->daddr.mac);
>         if (likely(!rc)) {
> -               llc_conn_send_pdu(sk, skb);
> +               rc = llc_conn_send_pdu(sk, skb);
>                 llc_conn_ac_inc_vs_by_1(sk, skb);
>         }
>         return rc;
> @@ -935,14 +935,17 @@ static int llc_conn_ac_send_i_rsp_f_set_ackpf(struct sock *sk,
>  int llc_conn_ac_send_i_as_ack(struct sock *sk, struct sk_buff *skb)
>  {
>         struct llc_sock *llc = llc_sk(sk);
> +       int ret;
>
>         if (llc->ack_must_be_send) {
> -               llc_conn_ac_send_i_rsp_f_set_ackpf(sk, skb);
> +               ret = llc_conn_ac_send_i_rsp_f_set_ackpf(sk, skb);
>                 llc->ack_must_be_send = 0 ;
>                 llc->ack_pf = 0;
> -       } else
> -               llc_conn_ac_send_i_cmd_p_set_0(sk, skb);
> -       return 0;
> +       } else {
> +               ret = llc_conn_ac_send_i_cmd_p_set_0(sk, skb);
> +       }
> +
> +       return ret;
>  }
>
>  /**
> diff --git a/net/llc/llc_conn.c b/net/llc/llc_conn.c
> index 9177dbb16dce..110e32bcb399 100644
> --- a/net/llc/llc_conn.c
> +++ b/net/llc/llc_conn.c
> @@ -30,7 +30,7 @@
>  #endif
>
>  static int llc_find_offset(int state, int ev_type);
> -static void llc_conn_send_pdus(struct sock *sk);
> +static int llc_conn_send_pdus(struct sock *sk, struct sk_buff *skb);
>  static int llc_conn_service(struct sock *sk, struct sk_buff *skb);
>  static int llc_exec_conn_trans_actions(struct sock *sk,
>                                        struct llc_conn_state_trans *trans,
> @@ -193,11 +193,11 @@ int llc_conn_state_process(struct sock *sk, struct sk_buff *skb)
>         return rc;
>  }
>
> -void llc_conn_send_pdu(struct sock *sk, struct sk_buff *skb)
> +int llc_conn_send_pdu(struct sock *sk, struct sk_buff *skb)
>  {
>         /* queue PDU to send to MAC layer */
>         skb_queue_tail(&sk->sk_write_queue, skb);
> -       llc_conn_send_pdus(sk);
> +       return llc_conn_send_pdus(sk, skb);
>  }
>
>  /**
> @@ -255,7 +255,7 @@ void llc_conn_resend_i_pdu_as_cmd(struct sock *sk, u8 nr, u8 first_p_bit)
>         if (howmany_resend > 0)
>                 llc->vS = (llc->vS + 1) % LLC_2_SEQ_NBR_MODULO;
>         /* any PDUs to re-send are queued up; start sending to MAC */
> -       llc_conn_send_pdus(sk);
> +       llc_conn_send_pdus(sk, NULL);
>  out:;
>  }
>
> @@ -296,7 +296,7 @@ void llc_conn_resend_i_pdu_as_rsp(struct sock *sk, u8 nr, u8 first_f_bit)
>         if (howmany_resend > 0)
>                 llc->vS = (llc->vS + 1) % LLC_2_SEQ_NBR_MODULO;
>         /* any PDUs to re-send are queued up; start sending to MAC */
> -       llc_conn_send_pdus(sk);
> +       llc_conn_send_pdus(sk, NULL);
>  out:;
>  }
>
> @@ -340,12 +340,16 @@ int llc_conn_remove_acked_pdus(struct sock *sk, u8 nr, u16 *how_many_unacked)
>  /**
>   *     llc_conn_send_pdus - Sends queued PDUs
>   *     @sk: active connection
> + *     @hold_skb: the skb held by caller, or NULL if does not care
>   *
> - *     Sends queued pdus to MAC layer for transmission.
> + *     Sends queued pdus to MAC layer for transmission. When @hold_skb is
> + *     NULL, always return 0. Otherwise, return 0 if @hold_skb is sent
> + *     successfully, or 1 for failure.
>   */
> -static void llc_conn_send_pdus(struct sock *sk)
> +static int llc_conn_send_pdus(struct sock *sk, struct sk_buff *hold_skb)
>  {
>         struct sk_buff *skb;
> +       int ret = 0;
>
>         while ((skb = skb_dequeue(&sk->sk_write_queue)) != NULL) {
>                 struct llc_pdu_sn *pdu = llc_pdu_sn_hdr(skb);
> @@ -357,10 +361,20 @@ static void llc_conn_send_pdus(struct sock *sk)
>                         skb_queue_tail(&llc_sk(sk)->pdu_unack_q, skb);
>                         if (!skb2)
>                                 break;
> -                       skb = skb2;
> +                       dev_queue_xmit(skb2);
> +               } else {
> +                       bool is_target = skb == hold_skb;
> +                       int rc;
> +
> +                       if (is_target)
> +                               skb_get(skb);
> +                       rc = dev_queue_xmit(skb);
> +                       if (is_target)
> +                               ret = rc;
>                 }
> -               dev_queue_xmit(skb);
>         }
> +
> +       return ret;
>  }
>
>  /**
> --
> 2.13.0
>



-- 

Thanks,
Noam Rathaus
Beyond Security

PGP Key ID: 7EF920D3C045D63F (Exp 2019-03)

Re: [Patch net] llc: properly handle dev_queue_xmit() return value

[David Miller]

From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Mon, 26 Mar 2018 15:08:33 -0700

> llc_conn_send_pdu() pushes the skb into write queue and
> calls llc_conn_send_pdus() to flush them out. However, the
> status of dev_queue_xmit() is not returned to caller,
> in this case, llc_conn_state_process().
> 
> llc_conn_state_process() needs hold the skb no matter
> success or failure, because it still uses it after that,
> therefore we should hold skb before dev_queue_xmit() when
> that skb is the one being processed by llc_conn_state_process().
> 
> For other callers, they can just pass NULL and ignore
> the return value as they are.
> 
> Reported-by: Noam Rathaus <noamr@beyondsecurity.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

Applied, thanks Cong.

Re: [Patch net] llc: properly handle dev_queue_xmit() return value

[David Miller]

From: Noam Rathaus <noamr@beyondsecurity.com>
Date: Tue, 27 Mar 2018 16:27:49 +0000

> Guys please fill me in on the next step?
> 
> If it’s applied it means it’s part of the official code of the kernel now?

It means it is in my networking GIT tree and will make it's way to Linus
in the not so distant future.

Re: [Patch net] llc: properly handle dev_queue_xmit() return value

[Noam Rathaus]

Hi,

Will you notify me when its been accepted? if not, how can I do this
checking myself to see if it was accepted?

On Tue, Mar 27, 2018 at 8:13 PM, David Miller <davem@davemloft.net> wrote:
> From: Noam Rathaus <noamr@beyondsecurity.com>
> Date: Tue, 27 Mar 2018 16:27:49 +0000
>
>> Guys please fill me in on the next step?
>>
>> If it’s applied it means it’s part of the official code of the kernel now?
>
> It means it is in my networking GIT tree and will make it's way to Linus
> in the not so distant future.



-- 

Thanks,
Noam Rathaus
Beyond Security

PGP Key ID: 7EF920D3C045D63F (Exp 2019-03)

Re: [Patch net] llc: properly handle dev_queue_xmit() return value

[Noam Rathaus]

Hi,

Is there any update?

On Fri, Apr 13, 2018 at 7:49 PM, Noam Rathaus <noamr@beyondsecurity.com> wrote:
> Hi
>
> Any update?
>
> On Thu, 29 Mar 2018 at 14:11, Noam Rathaus <noamr@beyondsecurity.com> wrote:
>>
>> Hi,
>>
>> Will you notify me when its been accepted? if not, how can I do this
>> checking myself to see if it was accepted?
>>
>> On Tue, Mar 27, 2018 at 8:13 PM, David Miller <davem@davemloft.net> wrote:
>> > From: Noam Rathaus <noamr@beyondsecurity.com>
>> > Date: Tue, 27 Mar 2018 16:27:49 +0000
>> >
>> >> Guys please fill me in on the next step?
>> >>
>> >> If it’s applied it means it’s part of the official code of the kernel
>> >> now?
>> >
>> > It means it is in my networking GIT tree and will make it's way to Linus
>> > in the not so distant future.
>>
>>
>>
>> --
>>
>> Thanks,
>> Noam Rathaus
>> Beyond Security
>>
>> PGP Key ID: 7EF920D3C045D63F (Exp 2019-03)
>
> --
> Thanks,
> Noam Rathaus



-- 

Thanks,
Noam Rathaus
Beyond Security

PGP Key ID: 7EF920D3C045D63F (Exp 2019-03)