| Message ID | 20180326193121.GA78356@davejwatson-mba.local |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] strparser: Fix sign of err codes | expand |
On Mon, Mar 26, 2018 at 12:31 PM, Dave Watson <davejwatson@fb.com> wrote: > 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. > Nice catch! It might be nice to have a comment at strp_parser_err and abort_parser description in Documentation/networking/strparser.txt should also be updated that err is a negative error value. Tom > Fixes: 43a0c6751a322847 ("strparser: Stream parser for messages") > Signed-off-by: Dave Watson <davejwatson@fb.com> > --- > net/strparser/strparser.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/net/strparser/strparser.c b/net/strparser/strparser.c > index 1fdab5c..b9283ce 100644 > --- a/net/strparser/strparser.c > +++ b/net/strparser/strparser.c > @@ -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); > } > } > @@ -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 >
On 03/26/18 01:44 PM, Tom Herbert wrote: > On Mon, Mar 26, 2018 at 12:31 PM, Dave Watson <davejwatson@fb.com> wrote: > > 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. > > > Nice catch! > > It might be nice to have a comment at strp_parser_err and abort_parser > description in Documentation/networking/strparser.txt should also be > updated that err is a negative error value. Sure I can update the docs also.
diff --git a/net/strparser/strparser.c b/net/strparser/strparser.c index 1fdab5c..b9283ce 100644 --- a/net/strparser/strparser.c +++ b/net/strparser/strparser.c @@ -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); } } @@ -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); }
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> --- net/strparser/strparser.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)