diff mbox series

[v2,net] strparser: Fix sign of err codes

Message ID 20180327152352.GA35552@davejwatson-mba.local
State Changes Requested, archived
Delegated to: David Miller
Headers show
Series [v2,net] strparser: Fix sign of err codes | expand

Commit Message

Dave Watson March 27, 2018, 3:23 p.m. UTC 
strp_parser_err is called with a negative code everywhere, which then
calls abort_parser with a negative code.  strp_msg_timeout calls
abort_parser directly with a positive code.  Negate ETIMEDOUT
to match signed-ness of other calls.

The default abort_parser callback, strp_abort_strp, sets
sk->sk_err to err.  Also negate the error here so sk_err always
holds a positive value, as the rest of the net code expects.  Currently
a negative sk_err can result in endless loops, or user code that
thinks it actually sent/received err bytes.

Found while testing net/tls_sw recv path.

Fixes: 43a0c6751a322847 ("strparser: Stream parser for messages")
Signed-off-by: Dave Watson <davejwatson@fb.com>
---
 Documentation/networking/strparser.txt | 5 +++--
 net/strparser/strparser.c              | 8 ++++----
 2 files changed, 7 insertions(+), 6 deletions(-)

Comments

David Miller March 27, 2018, 5:29 p.m. UTC | #1 
From: Dave Watson <davejwatson@fb.com>
Date: Tue, 27 Mar 2018 08:23:52 -0700

> strp_parser_err is called with a negative code everywhere, which then
> calls abort_parser with a negative code.  strp_msg_timeout calls
> abort_parser directly with a positive code.  Negate ETIMEDOUT
> to match signed-ness of other calls.
> 
> The default abort_parser callback, strp_abort_strp, sets
> sk->sk_err to err.  Also negate the error here so sk_err always
> holds a positive value, as the rest of the net code expects.  Currently
> a negative sk_err can result in endless loops, or user code that
> thinks it actually sent/received err bytes.
> 
> Found while testing net/tls_sw recv path.
> 
> Fixes: 43a0c6751a322847 ("strparser: Stream parser for messages")
> Signed-off-by: Dave Watson <davejwatson@fb.com>

Your v1 patch was already applied to my net tree, so you'll have to
send any further changes as a relative patch.
diff mbox series

Patch

diff --git a/Documentation/networking/strparser.txt b/Documentation/networking/strparser.txt
index 13081b3..23bf827 100644
--- a/Documentation/networking/strparser.txt
+++ b/Documentation/networking/strparser.txt
@@ -158,7 +158,8 @@  int (*read_sock_done)(struct strparser *strp, int err);
      the TCP socket in receive callback mode. The stream parser may
      read multiple messages in a loop and this function allows cleanup
      to occur when exiting the loop. If the callback is not set (NULL
-     in strp_init) a default function is used.
+     in strp_init) a default function is used.  err is a negative
+     error value.
 
 void (*abort_parser)(struct strparser *strp, int err);
 
@@ -166,7 +167,7 @@  void (*abort_parser)(struct strparser *strp, int err);
      in parsing. The default function stops the stream parser and
      sets the error in the socket if the parser is in receive callback
      mode. The default function can be changed by setting the callback
-     to non-NULL in strp_init.
+     to non-NULL in strp_init. err is a negative error value.
 
 Statistics
 ==========
diff --git a/net/strparser/strparser.c b/net/strparser/strparser.c
index 1fdab5c..a82ca09 100644
--- a/net/strparser/strparser.c
+++ b/net/strparser/strparser.c
@@ -44,7 +44,7 @@  static inline struct _strp_msg *_strp_msg(struct sk_buff *skb)
 		offsetof(struct qdisc_skb_cb, data));
 }
 
-/* Lower lock held */
+/* Lower lock held.  err is a negative error value. */
 static void strp_abort_strp(struct strparser *strp, int err)
 {
 	/* Unrecoverable error in receive */
@@ -60,7 +60,7 @@  static void strp_abort_strp(struct strparser *strp, int err)
 		struct sock *sk = strp->sk;
 
 		/* Report an error on the lower socket */
-		sk->sk_err = err;
+		sk->sk_err = -err;
 		sk->sk_error_report(sk);
 	}
 }
@@ -71,7 +71,7 @@  static void strp_start_timer(struct strparser *strp, long timeo)
 		mod_delayed_work(strp_wq, &strp->msg_timer_work, timeo);
 }
 
-/* Lower lock held */
+/* Lower lock held.  err is a negative error value. */
 static void strp_parser_err(struct strparser *strp, int err,
 			    read_descriptor_t *desc)
 {
@@ -458,7 +458,7 @@  static void strp_msg_timeout(struct work_struct *w)
 	/* Message assembly timed out */
 	STRP_STATS_INCR(strp->stats.msg_timeouts);
 	strp->cb.lock(strp);
-	strp->cb.abort_parser(strp, ETIMEDOUT);
+	strp->cb.abort_parser(strp, -ETIMEDOUT);
 	strp->cb.unlock(strp);
 }