Re: [PATCH v2 net] strparser: Fix sign of err codes

[David Miller]

From: Dave Watson 
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 

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

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

[Dave Watson]

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 
---
 Documentation/networking/strparser.txt | 5 +++--
 net/strparser/strparser.c  | 8 
 2 files changed, 7 insertions(+), 6 deletions(-)

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, >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);
 }
 
-- 
2.9.5