[PATCH net v4] sctp: socket: refactor sctp_skb_recv_datagram to use ERR_PTR

[luoqing]

From: Qing Luo <luoqing@xxxxxxxxxx>

The err output parameter in sctp_skb_recv_datagram() is passed to
callers but never validated, making error reporting unreliable.
Remove it and use ERR_PTR to encode errors directly in the return
value, which is the standard kernel pattern for this case.

Signed-off-by: Qing Luo <luoqing@xxxxxxxxxx>
---
 include/net/sctp/sctp.h |  2 +-
 net/sctp/socket.c       | 20 ++++++++++----------
 net/sctp/ulpevent.c     |  5 ++---
 3 files changed, 13 insertions(+), 14 deletions(-)

diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
index d50c27812504..b86d50d6b146 100644
--- a/include/net/sctp/sctp.h
+++ b/include/net/sctp/sctp.h
@@ -97,7 +97,7 @@ void sctp_sock_rfree(struct sk_buff *skb);
 
 extern struct percpu_counter sctp_sockets_allocated;
 int sctp_asconf_mgmt(struct sctp_sock *, struct sctp_sockaddr_entry *);
-struct sk_buff *sctp_skb_recv_datagram(struct sock *, int, int *);
+struct sk_buff *sctp_skb_recv_datagram(struct sock *sk, int flags);
 
 typedef int (*sctp_callback_t)(struct sctp_endpoint *, struct sctp_transport *, void *);
 void sctp_transport_walk_start(struct rhashtable_iter *iter);
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index c7b9e325ec1c..2deaa498e6cf 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -2123,9 +2123,11 @@ static int sctp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
 		goto out;
 	}
 
-	skb = sctp_skb_recv_datagram(sk, flags, &err);
-	if (!skb)
+	skb = sctp_skb_recv_datagram(sk, flags);
+	if (IS_ERR(skb)) {
+		err = PTR_ERR(skb);
 		goto out;
+	}
 
 	/* Get the total length of the skb including any skb's in the
 	 * frag_list.
@@ -9082,7 +9084,7 @@ static int sctp_wait_for_packet(struct sock *sk, int *err, long *timeo_p)
  * Note: This is pretty much the same routine as in core/datagram.c
  * with a few changes to make lksctp work.
  */
-struct sk_buff *sctp_skb_recv_datagram(struct sock *sk, int flags, int *err)
+struct sk_buff *sctp_skb_recv_datagram(struct sock *sk, int flags)
 {
 	int error;
 	struct sk_buff *skb;
@@ -9117,21 +9119,19 @@ struct sk_buff *sctp_skb_recv_datagram(struct sock *sk, int flags, int *err)
 		if (error)
 			goto no_packet;
 
-		if (sk->sk_shutdown & RCV_SHUTDOWN)
+		if (sk->sk_shutdown & RCV_SHUTDOWN) {
+			error = 0;
 			break;
-
+		}
 
 		/* User doesn't want to wait.  */
 		error = -EAGAIN;
 		if (!timeo)
 			goto no_packet;
-	} while (sctp_wait_for_packet(sk, err, &timeo) == 0);
-
-	return NULL;
+	} while (sctp_wait_for_packet(sk, &error, &timeo) == 0);
 
 no_packet:
-	*err = error;
-	return NULL;
+	return ERR_PTR(error);
 }
 
 /* If sndbuf has changed, wake up per association sndbuf waiters.  */
diff --git a/net/sctp/ulpevent.c b/net/sctp/ulpevent.c
index 8920ca92a011..21ae0adbaeef 100644
--- a/net/sctp/ulpevent.c
+++ b/net/sctp/ulpevent.c
@@ -1061,10 +1061,9 @@ void sctp_ulpevent_read_nxtinfo(const struct sctp_ulpevent *event,
 				struct sock *sk)
 {
 	struct sk_buff *skb;
-	int err;
 
-	skb = sctp_skb_recv_datagram(sk, MSG_PEEK | MSG_DONTWAIT, &err);
-	if (skb != NULL) {
+	skb = sctp_skb_recv_datagram(sk, MSG_PEEK | MSG_DONTWAIT);
+	if (!IS_ERR_OR_NULL(skb)) {
 		__sctp_ulpevent_read_nxtinfo(sctp_skb2event(skb),
 					     msghdr, skb);
 		/* Just release refcount here. */
-- 
2.25.1
>> I think it's used at [1] in sctp_recvmsg():
>>
>>         skb = sctp_skb_recv_datagram(sk, flags, &err);
>>         if (!skb)
>>                 goto out;
> Would it make more sense to ERR_PTR() etc ?
>
>
> 	David

Yes, you are right. The current implementation returns a negative error
code directly via err, but it would be cleaner to use ERR_PTR() to
unify the error path with other datagram receivers.

I will refactor this part in v4:
>
>
>> ...
>>
>> out:
>>         release_sock(sk);
>>         return err;   <------ [1]