[stable backport] Fix crash upon PSDBU report triggered on a multi-bu…

…ffer frame (#242) * Fix crash upon PSDBU report triggered on a multi-buffer frame * test: add test to make sure there's no PSDBU crash
travelping · Apr 22, 2022 · d42b87f · d42b87f
1 parent f37380c
commit d42b87f
Show file tree

Hide file tree

Showing 5 changed files with 28 additions and 5 deletions.
diff --git a/test/e2e/traffic/udp.go b/test/e2e/traffic/udp.go
@@ -40,6 +40,7 @@ type UDPPingConfig struct {
 	Retry          bool
 	Delay          time.Duration
 	NoMTUDiscovery bool
+	Burst          int
 }
 
 var _ TrafficConfig = &UDPPingConfig{}
@@ -226,8 +227,14 @@ func (up *UDPPing) Run(ctx context.Context, ns *network.NetNS) error {
 	recvBuf := make([]byte, up.cfg.PacketSize)
 	for i := 0; i < up.cfg.PacketCount; i++ {
 		up.genUDPPacket(i, sendBuf)
-		if _, err := c.Write(sendBuf); err != nil {
-			return errors.Wrap(err, "udp send")
+		n := 1
+		if up.cfg.Burst > 1 {
+			n = up.cfg.Burst
+		}
+		for i := 0; i < n; i++ {
+			if _, err := c.Write(sendBuf); err != nil {
+				return errors.Wrap(err, "udp send")
+			}
 		}
 		up.rec.RecordStats(TrafficStats{ClientSent: len(sendBuf)})
 		c.SetReadDeadline(time.Now().Add(up.cfg.Timeout))

diff --git a/test/e2e/upg_e2e.go b/test/e2e/upg_e2e.go
@@ -1092,6 +1092,7 @@ var _ = ginkgo.Describe("[Reporting]", func() {
 				PacketCount: 180, // 30s, but will be stopped when VPP exits
 				Retry:       true,
 				Delay:       100 * time.Millisecond,
+				Burst:       100,
 			}, &traffic.SimpleTrafficRec{})
 			tg.Start(f.Context, clientNS, serverNS)
 
@@ -1100,7 +1101,7 @@ var _ = ginkgo.Describe("[Reporting]", func() {
 			gomega.Expect(now.Before(sessionCfg.MonitoringTime)).To(gomega.BeTrue())
 			time.Sleep(sessionCfg.MonitoringTime.Add(2 * time.Second).Sub(now))
 
-			ginkgo.By("Updating monitoring time in the session (1)")
+			ginkgo.By("Updating monitoring time in the session")
 			// note that measurement period is reset here, and we want the new
 			// monitoring time to be after the report
 			sessionCfg.MonitoringTime = time.Now().Add(2 * time.Second).Truncate(time.Second)

diff --git a/upf/upf_pfcp.c b/upf/upf_pfcp.c
@@ -2441,6 +2441,7 @@ process_urrs (vlib_main_t * vm, upf_session_t * sess,
 	(upf_event_urr_hdr_t *) vec_header (uev,
 					    sizeof (upf_event_urr_hdr_t));
       ueh->session_idx = (uword) (sess - gtm->sessions);
+      ueh->cp_seid = sess->cp_seid;
       ueh->ue = tt.ip;
       ueh->status = status;
 

diff --git a/upf/upf_pfcp_server.c b/upf/upf_pfcp_server.c
@@ -1210,9 +1210,21 @@ static uword
 		  (upf_event_urr_hdr_t *) vec_header (uev,
 						      sizeof
 						      (upf_event_urr_hdr_t));
-		upf_session_t *sx;
+		upf_session_t *sx = 0;
+
+		/*
+		 * The session could have been freed or replaced after
+		 * this even has been queued, but before it has been
+		 * handled. So we check if the pool entry is free, and
+		 * also verify that CP-SEID in the session matches
+		 * CP-SEID in the event.
+		 */
+		if (!pool_is_free_index (gtm->sessions, ueh->session_idx))
+		  sx = pool_elt_at_index (gtm->sessions, ueh->session_idx);
+
+		if (!sx || sx->cp_seid != ueh->cp_seid)
+		  goto next_ev_urr;
 
-		sx = pool_elt_at_index (gtm->sessions, ueh->session_idx);
 		if (!(ueh->status & URR_DROP_SESSION))
 		  {
 		    upf_debug
@@ -1236,6 +1248,7 @@ static uword
 		    pfcp_free_session (sx);
 		  }
 
+	      next_ev_urr:
 		vec_free_h (uev, sizeof (upf_event_urr_hdr_t));
 	      }
 	    break;

diff --git a/upf/upf_pfcp_server.h b/upf/upf_pfcp_server.h
@@ -110,6 +110,7 @@ typedef struct
 typedef struct
 {
   uword session_idx;
+  u64 cp_seid;
   ip46_address_t ue;
   u8 status;
 } upf_event_urr_hdr_t;