[SCTP] Fix for panic on recvmsg() with MSG_PEEK flag and some ulpevent

       cleanup.

include/net/sctp/ulpevent.h

@@ -40,6 +40,7 @@
  *   Jon Grimm             <jgrimm@us.ibm.com>
  *   La Monte H.P. Yarroll <piggy@acm.org>
  *   Karl Knutson          <karl@athena.chicago.il.us>
+ *   Sridhar Samudrala     <sri@us.ibm.com>
  *
  * Any bugs reported given to us we will try to fix... any fixes shared will
  * be incorporated into the next SCTP release.
@@ -72,9 +73,8 @@ static inline struct sctp_ulpevent *sctp_skb2event(struct sk_buff *skb)
 }
 
 struct sctp_ulpevent *sctp_ulpevent_new(int size, int flags, int gfp);
-struct sctp_ulpevent *sctp_ulpevent_init(struct sctp_ulpevent *, int flags);
+void sctp_ulpevent_init(struct sctp_ulpevent *, int flags);
 void sctp_ulpevent_free(struct sctp_ulpevent *);
-void sctp_ulpevent_kfree_skb(struct sk_buff *skb);
 int sctp_ulpevent_is_notification(const struct sctp_ulpevent *);
 void sctp_queue_purge_ulpevents(struct sk_buff_head *list);
 

net/sctp/socket.c

@@ -1342,8 +1342,7 @@ SCTP_STATIC int sctp_recvmsg(struct kiocb *iocb, struct sock *sk,
 
 		/* When only partial message is copied to the user, increase
 		 * rwnd by that amount. If all the data in the skb is read,
-		 * rwnd is updated when the skb's destructor is called via
-		 * sctp_ulpevent_free().
+		 * rwnd is updated when the event is freed.
 		 */
 		sctp_assoc_rwnd_increase(event->asoc, copied);
 		goto out;
@@ -1354,7 +1353,18 @@ SCTP_STATIC int sctp_recvmsg(struct kiocb *iocb, struct sock *sk,
 		msg->msg_flags &= ~MSG_EOR;
 
 out_free:
-	sctp_ulpevent_kfree_skb(skb); /* Free the skb. */
+	if (flags & MSG_PEEK) {
+		/* Release the skb reference acquired after peeking the skb in
+		 * sctp_skb_recv_datagram().
+		 */
+		kfree_skb(skb);
+	} else {
+		/* Free the event which includes releasing the reference to
+		 * the owner of the skb, freeing the skb and updating the
+		 * rwnd.
+		 */ 
+		sctp_ulpevent_free(event);
+	}
 out:
 	sctp_release_sock(sk);
 	return err;

net/sctp/ulpevent.c

@@ -35,6 +35,7 @@
  * Written or modified by:
  *    Jon Grimm             <jgrimm@us.ibm.com>
  *    La Monte H.P. Yarroll <piggy@acm.org>
+ *    Sridhar Samudrala     <sri@us.ibm.com>
  *
  * Any bugs reported given to us we will try to fix... any fixes shared will
  * be incorporated into the next SCTP release.
@@ -46,10 +47,12 @@
 #include <net/sctp/sctp.h>
 #include <net/sctp/sm.h>
 
-static void sctp_ulpevent_set_owner_r(struct sk_buff *skb,
-				      struct sctp_association *asoc);
-static void sctp_ulpevent_set_owner(struct sk_buff *skb,
+static inline void sctp_ulpevent_set_owner(struct sctp_ulpevent *event,
 					   const struct sctp_association *asoc);
+static inline void sctp_ulpevent_release_owner(struct sctp_ulpevent *event);
+static void sctp_ulpevent_receive_data(struct sctp_ulpevent *event,
+				       struct sctp_association *asoc);
+static void sctp_ulpevent_release_data(struct sctp_ulpevent *event);
 
 /* Create a new sctp_ulpevent.  */
 struct sctp_ulpevent *sctp_ulpevent_new(int size, int msg_flags, int gfp)
@@ -62,31 +65,19 @@ struct sctp_ulpevent *sctp_ulpevent_new(int size, int msg_flags, int gfp)
 		goto fail;
 
 	event = sctp_skb2event(skb);
-	event = sctp_ulpevent_init(event, msg_flags);
-	if (!event)
-		goto fail_init;
+	sctp_ulpevent_init(event, msg_flags);
+
 	return event;
 
-fail_init:
-	kfree_skb(skb);
 fail:
 	return NULL;
 }
 
 /* Initialize an ULP event from an given skb.  */
-struct sctp_ulpevent *sctp_ulpevent_init(struct sctp_ulpevent *event,
-					 int msg_flags)
+void sctp_ulpevent_init(struct sctp_ulpevent *event, int msg_flags)
 {
 	memset(event, sizeof(struct sctp_ulpevent), 0x00);
 	event->msg_flags = msg_flags;
-	return event;
-}
-
-
-/* Dispose of an event.  */
-void sctp_ulpevent_free(struct sctp_ulpevent *event)
-{
-	kfree_skb(sctp_event2skb(event));
 }
 
 /* Is this a MSG_NOTIFICATION?  */
@@ -190,7 +181,7 @@ struct sctp_ulpevent  *sctp_ulpevent_make_assoc_change(
 	 * All notifications for a given association have the same association
 	 * identifier.  For TCP style socket, this field is ignored.
 	 */
-	sctp_ulpevent_set_owner(skb, asoc);
+	sctp_ulpevent_set_owner(event, asoc);
 	sac->sac_assoc_id = sctp_assoc2id(asoc);
 
 	return event;
@@ -282,7 +273,7 @@ struct sctp_ulpevent *sctp_ulpevent_make_peer_addr_change(
 	 * All notifications for a given association have the same association
 	 * identifier.  For TCP style socket, this field is ignored.
 	 */
-	sctp_ulpevent_set_owner(skb, asoc);
+	sctp_ulpevent_set_owner(event, asoc);
 	spc->spc_assoc_id = sctp_assoc2id(asoc);
 
 	/* Sockets API Extensions for SCTP
@@ -347,10 +338,7 @@ struct sctp_ulpevent *sctp_ulpevent_make_remote_error(
 
 	/* Embed the event fields inside the cloned skb.  */
 	event = sctp_skb2event(skb);
-	event = sctp_ulpevent_init(event, MSG_NOTIFICATION);
-
-	if (!event)
-		goto fail;
+	sctp_ulpevent_init(event, MSG_NOTIFICATION);
 
 	sre = (struct sctp_remote_error *)
 		skb_push(skb, sizeof(struct sctp_remote_error));
@@ -403,8 +391,7 @@ struct sctp_ulpevent *sctp_ulpevent_make_remote_error(
 	 * All notifications for a given association have the same association
 	 * identifier.  For TCP style socket, this field is ignored.
 	 */
-	skb = sctp_event2skb(event);
-	sctp_ulpevent_set_owner(skb, asoc);
+	sctp_ulpevent_set_owner(event, asoc);
 	sre->sre_assoc_id = sctp_assoc2id(asoc);
 
 	return event;
@@ -443,9 +430,7 @@ struct sctp_ulpevent *sctp_ulpevent_make_send_failed(
 
 	/* Embed the event fields inside the cloned skb.  */
 	event = sctp_skb2event(skb);
-	event = sctp_ulpevent_init(event, MSG_NOTIFICATION);
-	if (!event)
-		goto fail;
+	sctp_ulpevent_init(event, MSG_NOTIFICATION);
 
 	ssf = (struct sctp_send_failed *)
 		skb_push(skb, sizeof(struct sctp_send_failed));
@@ -516,8 +501,7 @@ struct sctp_ulpevent *sctp_ulpevent_make_send_failed(
 	 * same association identifier.  For TCP style socket, this field is
 	 * ignored.
 	 */
-	skb = sctp_event2skb(event);
-	sctp_ulpevent_set_owner(skb, asoc);
+	sctp_ulpevent_set_owner(event, asoc);
 	ssf->ssf_assoc_id = sctp_assoc2id(asoc);
 	return event;
 
@@ -580,7 +564,7 @@ struct sctp_ulpevent *sctp_ulpevent_make_shutdown_event(
 	 * All notifications for a given association have the same association
 	 * identifier.  For TCP style socket, this field is ignored.
 	 */
-	sctp_ulpevent_set_owner(skb, asoc);
+	sctp_ulpevent_set_owner(event, asoc);
 	sse->sse_assoc_id = sctp_assoc2id(asoc);
 
 	return event;
@@ -602,7 +586,7 @@ struct sctp_ulpevent *sctp_ulpevent_make_rcvmsg(struct sctp_association *asoc,
 {
 	struct sctp_ulpevent *event;
 	struct sctp_sndrcvinfo *info;
-	struct sk_buff *skb, *list;
+	struct sk_buff *skb;
 	size_t padding, len;
 
 	/* Clone the original skb, sharing the data.  */
@@ -628,24 +612,15 @@ struct sctp_ulpevent *sctp_ulpevent_make_rcvmsg(struct sctp_association *asoc,
 	/* Fixup cloned skb with just this chunks data.  */
 	skb_trim(skb, chunk->chunk_end - padding - skb->data);
 
-	/* Set up a destructor to do rwnd accounting.  */
-	sctp_ulpevent_set_owner_r(skb, asoc);
-
 	/* Embed the event fields inside the cloned skb.  */
 	event = sctp_skb2event(skb);
 
 	/* Initialize event with flags 0.  */
-	event = sctp_ulpevent_init(event, 0);
-	if (!event)
-		goto fail_init;
+	sctp_ulpevent_init(event, 0);
 
 	event->iif = sctp_chunk_iif(chunk);
-	/* Note:  Not clearing the entire event struct as
-	 * this is just a fragment of the real event.  However,
-	 * we still need to do rwnd accounting.
-	 */
-	for (list = skb_shinfo(skb)->frag_list; list; list = list->next)
-		sctp_ulpevent_set_owner_r(list, asoc);
+
+	sctp_ulpevent_receive_data(event, asoc);
 
 	info = (struct sctp_sndrcvinfo *) &event->sndrcvinfo;
 
@@ -736,9 +711,6 @@ struct sctp_ulpevent *sctp_ulpevent_make_rcvmsg(struct sctp_association *asoc,
 
 	return event;
 
-fail_init:
-	kfree_skb(skb);
-
 fail:
 	return NULL;
 }
@@ -794,7 +766,7 @@ struct sctp_ulpevent *sctp_ulpevent_make_pdapi(
 	 *
 	 * The association id field, holds the identifier for the association.
 	 */
-	sctp_ulpevent_set_owner(skb, asoc);
+	sctp_ulpevent_set_owner(event, asoc);
 	pd->pdapi_assoc_id = sctp_assoc2id(asoc);
 
 	return event;
@@ -839,92 +811,95 @@ static void sctp_stub_rfree(struct sk_buff *skb)
  */
 }
 
-/* Do accounting for bytes just read by user.  */
-static void sctp_rcvmsg_rfree(struct sk_buff *skb)
-{
-	struct sctp_association *asoc;
-	struct sctp_ulpevent *event;
-	struct sk_buff *frag;
-
-	/* Current stack structures assume that the rcv buffer is
-	 * per socket.   For UDP style sockets this is not true as
-	 * multiple associations may be on a single UDP-style socket.
-	 * Use the local private area of the skb to track the owning
-	 * association.
+/* Hold the association in case the msg_name needs read out of
+ * the association.
  */
-
-	event = sctp_skb2event(skb);
-	asoc = event->asoc;
-	sctp_assoc_rwnd_increase(asoc, skb_headlen(skb));
-
-	/* Don't forget the fragments. */
-	for (frag = skb_shinfo(skb)->frag_list; frag; frag = frag->next) {
-		/* NOTE:  skb_shinfos are recursive. */
-		sctp_rcvmsg_rfree(frag);
-	}
-	sctp_association_put(asoc);
-}
-
-/* Charge receive window for bytes received.  */
-static void sctp_ulpevent_set_owner_r(struct sk_buff *skb,
-				      struct sctp_association *asoc)
+static inline void sctp_ulpevent_set_owner(struct sctp_ulpevent *event,
+					   const struct sctp_association *asoc)
 {
-	struct sctp_ulpevent *event;
+	struct sk_buff *skb;
 
-	/* The current stack structures assume that the rcv buffer is
-	 * per socket.  For UDP-style sockets this is not true as
-	 * multiple associations may be on a single UDP-style socket.
-	 * We use the local private area of the skb to track the owning
-	 * association.
+	/* Cast away the const, as we are just wanting to
+	 * bump the reference count.
 	 */
-	sctp_association_hold(asoc);
+	sctp_association_hold((struct sctp_association *)asoc);
+	skb = sctp_event2skb(event);
 	skb->sk = asoc->base.sk;
-	event = sctp_skb2event(skb);
-	event->asoc = asoc;
-
+	event->asoc = (struct sctp_association *)asoc;
 	skb->destructor = sctp_stub_rfree;
-	sctp_assoc_rwnd_decrease(asoc, skb_headlen(skb));
 }
 
 /* A simple destructor to give up the reference to the association. */
-static void sctp_ulpevent_rfree(struct sk_buff *skb)
+static inline void sctp_ulpevent_release_owner(struct sctp_ulpevent *event)
 {
-	struct sctp_ulpevent *event;
-	event = sctp_skb2event(skb);
 	sctp_association_put(event->asoc);
 }
 
-/* Hold the association in case the msg_name needs read out of
- * the association.
+/* Do accounting for bytes received and hold a reference to the association
+ * for each skb.
  */
-static void sctp_ulpevent_set_owner(struct sk_buff *skb,
-				    const struct sctp_association *asoc)
+static void sctp_ulpevent_receive_data(struct sctp_ulpevent *event,
+				       struct sctp_association *asoc)
 {
-	struct sctp_ulpevent *event;
+	struct sk_buff *skb, *frag;
 
-	/* Cast away the const, as we are just wanting to
-	 * bump the reference count.
+	skb = sctp_event2skb(event);
+	/* Set the owner and charge rwnd for bytes received.  */
+	sctp_ulpevent_set_owner(event, asoc);
+	sctp_assoc_rwnd_decrease(asoc, skb_headlen(skb));
+
+	/* Note:  Not clearing the entire event struct as this is just a
+	 * fragment of the real event.  However, we still need to do rwnd
+	 * accounting.
+	 * In general, the skb passed from IP can have only 1 level of
+	 * fragments. But we allow multiple levels of fragments. 
 	 */
-	sctp_association_hold((struct sctp_association *)asoc);
-	skb->sk = asoc->base.sk;
-	event = sctp_skb2event(skb);
-	event->asoc = (struct sctp_association *)asoc;
-	skb->destructor = sctp_stub_rfree;
+	for (frag = skb_shinfo(skb)->frag_list; frag; frag = frag->next) {
+		sctp_ulpevent_receive_data(sctp_skb2event(frag), asoc);
+	}
 }
 
-/* Free a ulpevent that has an owner.  See comments in
- * sctp_stub_rfree().
+/* Do accounting for bytes just read by user and release the references to
+ * the association.
  */ 
-void sctp_ulpevent_kfree_skb(struct sk_buff *skb)
+static void sctp_ulpevent_release_data(struct sctp_ulpevent *event)
 {
-	struct sctp_ulpevent *event;
+	struct sk_buff *skb, *frag;
 
-	event = sctp_skb2event(skb);
+	/* Current stack structures assume that the rcv buffer is
+	 * per socket.   For UDP style sockets this is not true as
+	 * multiple associations may be on a single UDP-style socket.
+	 * Use the local private area of the skb to track the owning
+	 * association.
+	 */
+
+	skb = sctp_event2skb(event);
+	sctp_assoc_rwnd_increase(event->asoc, skb_headlen(skb));
+
+	/* Don't forget the fragments. */
+	for (frag = skb_shinfo(skb)->frag_list; frag; frag = frag->next) {
+		/* NOTE:  skb_shinfos are recursive. Although IP returns
+		 * skb's with only 1 level of fragments, SCTP reassembly can
+		 * increase the levels.
+		 */
+		sctp_ulpevent_release_data(sctp_skb2event(frag));
+	}
+	sctp_ulpevent_release_owner(event);
+}
+
+/* Free a ulpevent that has an owner.  It includes releasing the reference
+ * to the owner, updating the rwnd in case of a DATA event and freeing the
+ * skb.
+ * See comments in sctp_stub_rfree().
+ */
+void sctp_ulpevent_free(struct sctp_ulpevent *event)
+{
 	if (sctp_ulpevent_is_notification(event))
-		sctp_ulpevent_rfree(skb);
+		sctp_ulpevent_release_owner(event);
 	else
-		sctp_rcvmsg_rfree(skb);
-	kfree_skb(skb);
+		sctp_ulpevent_release_data(event);
+
+	kfree_skb(sctp_event2skb(event));
 }
 
 /* Purge the skb lists holding ulpevents. */
@@ -932,5 +907,5 @@ void sctp_queue_purge_ulpevents(struct sk_buff_head *list)
 {
 	struct sk_buff *skb;
 	while ((skb = skb_dequeue(list)) != NULL)
-		sctp_ulpevent_kfree_skb(skb);
+		sctp_ulpevent_free(sctp_skb2event(skb));
 }

net/sctp/ulpqueue.c

@@ -99,12 +99,12 @@ void sctp_ulpq_flush(struct sctp_ulpq *ulpq)
 
 	while ((skb = __skb_dequeue(&ulpq->lobby))) {
 		event = sctp_skb2event(skb);
-		sctp_ulpevent_kfree_skb(skb);
+		sctp_ulpevent_free(event);
 	}
 
 	while ((skb = __skb_dequeue(&ulpq->reasm))) {
 		event = sctp_skb2event(skb);
-		sctp_ulpevent_kfree_skb(skb);
+		sctp_ulpevent_free(event);
 	}
 
 }
@@ -237,7 +237,7 @@ int sctp_ulpq_tail_event(struct sctp_ulpq *ulpq, struct sctp_ulpevent *event)
 	if (sctp_event2skb(event)->list)
 		sctp_queue_purge_ulpevents(sctp_event2skb(event)->list);
 	else
-		sctp_ulpevent_kfree_skb(sctp_event2skb(event));
+		sctp_ulpevent_free(event);
 	return 0;
 }
 
@@ -696,7 +696,7 @@ static __u16 sctp_ulpq_renege_order(struct sctp_ulpq *ulpq, __u16 needed)
 		event = sctp_skb2event(skb);
 		tsn = event->sndrcvinfo.sinfo_tsn;
 
-		sctp_ulpevent_kfree_skb(skb);
+		sctp_ulpevent_free(event);
 		sctp_tsnmap_renege(tsnmap, tsn);
 		if (freed >= needed)
 			return freed;
@@ -722,7 +722,7 @@ static __u16 sctp_ulpq_renege_frags(struct sctp_ulpq *ulpq, __u16 needed)
 		event = sctp_skb2event(skb);
 		tsn = event->sndrcvinfo.sinfo_tsn;
 
-		sctp_ulpevent_kfree_skb(skb);
+		sctp_ulpevent_free(event);
 		sctp_tsnmap_renege(tsnmap, tsn);
 		if (freed >= needed)
 			return freed;