more virtio-net csum hardening

The following patch will add ingress resolution of uncommon virtio net hdr checksum metadata (software checksum completion at ingress) which adds performance overhead to the packet processing.

We should make it visible, so a hypothetical workload relying on uncommon metadata can be distinguished from none at all. This way issues in the parsing will be reported and not quietly pay the host-CPU checksum overhead instead of offload.

Flip every call to virtio_net_hdr_to_skb to test for negative errno only, and have tun count these events per device as rx_csum_sw, exposed via ethtool -S.

Assisted-by LLM

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 drivers/net/tap.c        |  4 ++--
 drivers/net/tun.c        | 41 ++++++++++++++++++++++++++++++++++++++--
 drivers/net/virtio_net.c |  2 +-
 net/packet/af_packet.c   |  4 ++--
 4 files changed, 44 insertions(+), 7 deletions(-)

drivers/net/tap.c +1 -1

index ff67d99deb39..73e883394446 100644
@@ -649,7 +649,7 @@ static ssize_t tap_get_user(struct tap_queue *q, void *msg_control,
 
 	if (vnet_hdr_len) {
 		err = tun_vnet_hdr_to_skb(q->flags, skb, &vnet_hdr);
-		if (err) {
+		if (err < 0) {
 			rcu_read_unlock();
 			drop_reason = SKB_DROP_REASON_DEV_HDR;
 			goto err_kfree;

drivers/net/tun.c +0 -0

 	struct bpf_prog __rcu *xdp_prog;
 	struct tun_prog __rcu *steering_prog;
 	struct tun_prog __rcu *filter_prog;

@@ -1935,11 +1936,16 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
 		break;

drivers/net/tun.c +3 -0

+		atomic64_inc(&tun->rx_csum_sw);
+		err = 0;
+	}
 
 	if ((tun->flags & TUN_TYPE_MASK) == IFF_TAP)
 		skb->protocol = eth_type_trans(skb, tun->dev);

@@ -2605,12 +2611,15 @@ static int tun_xdp_one(struct tun_struct *tun,
 	tnl_hdr = (struct virtio_net_hdr_v1_hash_tunnel *)gso;
 	skb->dev = tun->dev;

drivers/net/tun.c +28 -0

 	skb->protocol = eth_type_trans(skb, tun->dev);
 	skb_reset_network_header(skb);

@@ -3739,7 +3748,35 @@ static void tun_get_channels(struct net_device *dev,
 	channels->max_combined = tun->flags & IFF_MULTI_QUEUE ? MAX_TAP_QUEUES : 1;
 }
 
+#define TUN_STATS_LEN	1
+static const char tun_ethtool_strings[TUN_STATS_LEN][ETH_GSTRING_LEN] = {
+	"rx_csum_sw",
+};
+
+static int tun_get_sset_count(struct net_device *dev, int sset)
+{
+	return sset == ETH_SS_STATS ? TUN_STATS_LEN : -EOPNOTSUPP;
+}
+
+static void tun_get_strings(struct net_device *dev, u32 stringset, u8 *data)
+{
+	if (stringset == ETH_SS_STATS)
+		memcpy(data, tun_ethtool_strings,
+		       sizeof(tun_ethtool_strings));
+}
+
+static void tun_get_ethtool_stats(struct net_device *dev,
+				  struct ethtool_stats *stats, u64 *data)
+{
+	struct tun_struct *tun = netdev_priv(dev);
+
+	data[0] = atomic64_read(&tun->rx_csum_sw);
+}
+
 static const struct ethtool_ops tun_ethtool_ops = {
+	.get_sset_count		= tun_get_sset_count,
+	.get_strings		= tun_get_strings,
+	.get_ethtool_stats	= tun_get_ethtool_stats,
 	.supported_coalesce_params = ETHTOOL_COALESCE_RX_MAX_FRAMES,
 	.get_drvinfo	= tun_get_drvinfo,
 	.get_msglevel	= tun_get_msglevel,

drivers/net/virtio_net.c +0 -0

index daab43ac92ce..2e6fd81df9c4 100644

net/packet/af_packet.c +2 -2

index 52ae3d875e97..e6aec9ecfa99 100644
@@ -2913,7 +2913,7 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
 		}
 
 		if (has_vnet_hdr) {
-			if (virtio_net_hdr_to_skb(skb, &vnet_hdr, vio_le())) {
+			if (virtio_net_hdr_to_skb(skb, &vnet_hdr, vio_le()) < 0) {
 				tp_len = -EINVAL;
 				goto tpacket_error;
 			}

@@ -3136,7 +3136,7 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
 
 	if (vnet_hdr_sz) {
 		err = virtio_net_hdr_to_skb(skb, &vnet_hdr, vio_le());
-		if (err)
+		if (err < 0)
 			goto out_free;
 		len += vnet_hdr_sz;
 	}

Signature · 1 line

MST

Quoted 36 lines · Michael S. Tsirkin

The following patch will add ingress resolution of uncommon virtio net hdr checksum metadata (software checksum completion at ingress) which adds performance overhead to the packet processing.

We should make it visible, so a hypothetical workload relying on uncommon metadata can be distinguished from none at all. This way issues in the parsing will be reported and not quietly pay the host-CPU checksum overhead instead of offload.

Flip every call to virtio_net_hdr_to_skb to test for negative errno only, and have tun count these events per device as rx_csum_sw, exposed via ethtool -S.

Assisted-by LLM

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 drivers/net/tap.c        |  4 ++--
 drivers/net/tun.c        | 41 ++++++++++++++++++++++++++++++++++++++--
 drivers/net/virtio_net.c |  2 +-
 net/packet/af_packet.c   |  4 ++--
 4 files changed, 44 insertions(+), 7 deletions(-)

drivers/net/tap.c +1 -1

index ff67d99deb39..73e883394446 100644
@@ -649,7 +649,7 @@ static ssize_t tap_get_user(struct tap_queue *q, void *msg_control,
 
 	if (vnet_hdr_len) {
 		err = tun_vnet_hdr_to_skb(q->flags, skb, &vnet_hdr);
-		if (err) {
+		if (err < 0) {
 			rcu_read_unlock();
 			drop_reason = SKB_DROP_REASON_DEV_HDR;
 			goto err_kfree;

drivers/net/tap.c static int tap_get_user_xdp(struct tap_queue *q, struct xdp_buff *xdp) · line 1090

Quoted 8 lines

drivers/net/tap.c +1 -1

@@ -1084,7 +1084,7 @@ static int tap_get_user_xdp(struct tap_queue *q, struct xdp_buff *xdp)
 
 	if (vnet_hdr_len) {
 		err = tun_vnet_hdr_to_skb(q->flags, skb, gso);
-		if (err) {
+		if (err < 0) {
 			rcu_read_unlock();
 			goto err_kfree;
 		}

similar to tun, should tap count this event?

drivers/net/tun.c struct tun_struct { · line 213

Quoted 9 lines

drivers/net/tun.c +1 -0

index 242899f7fd07..19cb443c89b0 100644
@@ -210,6 +210,7 @@ struct tun_struct {
 	u32 flow_count;
 	u32 rx_batched;
 	atomic_long_t rx_frame_errors;
+	atomic64_t rx_csum_sw;

is a normal (and cheaper on 32b platforms) atomic_t or atomic_long_t sufficient?

Quoted 5 lines

drivers/net/tun.c +0 -0

 	struct bpf_prog __rcu *xdp_prog;
 	struct tun_prog __rcu *steering_prog;
 	struct tun_prog __rcu *filter_prog;

@@ -1935,11 +1936,16 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
 		break;

drivers/net/tun.c static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile, · line 1945

Quoted 10 lines

drivers/net/tun.c +3 -1

 	}
 
-	if (tun_vnet_hdr_tnl_to_skb(tun->flags, features, skb, &hdr)) {
+	err = tun_vnet_hdr_tnl_to_skb(tun->flags, features, skb, &hdr);
+	if (err < 0) {
 		atomic_long_inc(&tun->rx_frame_errors);
 		err = -EINVAL;
 		goto free_skb;
 	}
+	if (err > 0) {

} else if (err > 0) {

Quoted 9 lines

drivers/net/tun.c +3 -0

+		atomic64_inc(&tun->rx_csum_sw);
+		err = 0;
+	}
 
 	if ((tun->flags & TUN_TYPE_MASK) == IFF_TAP)
 		skb->protocol = eth_type_trans(skb, tun->dev);

@@ -2605,12 +2611,15 @@ static int tun_xdp_one(struct tun_struct *tun,
 	tnl_hdr = (struct virtio_net_hdr_v1_hash_tunnel *)gso;
 	skb->dev = tun->dev;

drivers/net/tun.c static int tun_xdp_one(struct tun_struct *tun, · line 2621

Quoted 10 lines

drivers/net/tun.c +3 -1

 	skb_set_network_header(skb, ETH_HLEN);
-	if (tun_vnet_hdr_tnl_to_skb(tun->flags, features, skb, tnl_hdr)) {
+	ret = tun_vnet_hdr_tnl_to_skb(tun->flags, features, skb, tnl_hdr);
+	if (ret < 0) {
 		atomic_long_inc(&tun->rx_frame_errors);
 		kfree_skb(skb);
 		ret = -EINVAL;
 		goto out;
 	}
+	if (ret > 0)

same

drivers/net/tun.c static int tun_xdp_one(struct tun_struct *tun, · line 2622

Quoted 1 line

drivers/net/tun.c +1 -0

+		atomic64_inc(&tun->rx_csum_sw);

maybe reset ret = 0 in all these branches

Quoted 47 lines

drivers/net/tun.c +28 -0

 	skb->protocol = eth_type_trans(skb, tun->dev);
 	skb_reset_network_header(skb);

@@ -3739,7 +3748,35 @@ static void tun_get_channels(struct net_device *dev,
 	channels->max_combined = tun->flags & IFF_MULTI_QUEUE ? MAX_TAP_QUEUES : 1;
 }
 
+#define TUN_STATS_LEN	1
+static const char tun_ethtool_strings[TUN_STATS_LEN][ETH_GSTRING_LEN] = {
+	"rx_csum_sw",
+};
+
+static int tun_get_sset_count(struct net_device *dev, int sset)
+{
+	return sset == ETH_SS_STATS ? TUN_STATS_LEN : -EOPNOTSUPP;
+}
+
+static void tun_get_strings(struct net_device *dev, u32 stringset, u8 *data)
+{
+	if (stringset == ETH_SS_STATS)
+		memcpy(data, tun_ethtool_strings,
+		       sizeof(tun_ethtool_strings));
+}
+
+static void tun_get_ethtool_stats(struct net_device *dev,
+				  struct ethtool_stats *stats, u64 *data)
+{
+	struct tun_struct *tun = netdev_priv(dev);
+
+	data[0] = atomic64_read(&tun->rx_csum_sw);
+}
+
 static const struct ethtool_ops tun_ethtool_ops = {
+	.get_sset_count		= tun_get_sset_count,
+	.get_strings		= tun_get_strings,
+	.get_ethtool_stats	= tun_get_ethtool_stats,
 	.supported_coalesce_params = ETHTOOL_COALESCE_RX_MAX_FRAMES,
 	.get_drvinfo	= tun_get_drvinfo,
 	.get_msglevel	= tun_get_msglevel,

drivers/net/virtio_net.c +0 -0

index daab43ac92ce..2e6fd81df9c4 100644
@@ -2519,7 +2519,7 @@ static void virtnet_receive_done(struct virtnet_info *vi, struct receive_queue *
 	skb_set_network_header(skb, ETH_HLEN);
 	if (virtio_net_hdr_tnl_to_skb(skb, &hdr->tnl_hdr, vi->rx_tnl,
 				      vi->rx_tnl_csum,

drivers/net/virtio_net.c static void virtnet_receive_done(struct virtnet_info *vi, struct receive_queue * · line 2525

Quoted 5 lines

drivers/net/virtio_net.c +1 -1

-				      virtio_is_little_endian(vi->vdev))) {
+				      virtio_is_little_endian(vi->vdev)) < 0) {
 		net_warn_ratelimited("%s: bad gso: type: %x, size: %u, flags %x tunnel %d tnl csum %d\n",
 				     dev->name, hdr->hdr.gso_type,
 				     hdr->hdr.gso_size, hdr->hdr.flags,

should virtio_net count these too? Or do we assume that devices are more trustworthy, so unlikely to cause this issue?

Quoted 25 lines

net/packet/af_packet.c +2 -2

index 52ae3d875e97..e6aec9ecfa99 100644
@@ -2913,7 +2913,7 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
 		}
 
 		if (has_vnet_hdr) {
-			if (virtio_net_hdr_to_skb(skb, &vnet_hdr, vio_le())) {
+			if (virtio_net_hdr_to_skb(skb, &vnet_hdr, vio_le()) < 0) {
 				tp_len = -EINVAL;
 				goto tpacket_error;
 			}

@@ -3136,7 +3136,7 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
 
 	if (vnet_hdr_sz) {
 		err = virtio_net_hdr_to_skb(skb, &vnet_hdr, vio_le());
-		if (err)
+		if (err < 0)
 			goto out_free;
 		len += vnet_hdr_sz;
 	}

Signature · 1 line

MST

Quoted 48 lines · Willem de Bruijn

Quoted 36 lines · Michael S. Tsirkin

The following patch will add ingress resolution of uncommon virtio net hdr checksum metadata (software checksum completion at ingress) which adds performance overhead to the packet processing.

We should make it visible, so a hypothetical workload relying on uncommon metadata can be distinguished from none at all. This way issues in the parsing will be reported and not quietly pay the host-CPU checksum overhead instead of offload.

Flip every call to virtio_net_hdr_to_skb to test for negative errno only, and have tun count these events per device as rx_csum_sw, exposed via ethtool -S.

Assisted-by LLM

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 drivers/net/tap.c        |  4 ++--
 drivers/net/tun.c        | 41 ++++++++++++++++++++++++++++++++++++++--
 drivers/net/virtio_net.c |  2 +-
 net/packet/af_packet.c   |  4 ++--
 4 files changed, 44 insertions(+), 7 deletions(-)

drivers/net/tap.c +1 -1

index ff67d99deb39..73e883394446 100644
@@ -649,7 +649,7 @@ static ssize_t tap_get_user(struct tap_queue *q, void *msg_control,
 
 	if (vnet_hdr_len) {
 		err = tun_vnet_hdr_to_skb(q->flags, skb, &vnet_hdr);
-		if (err) {
+		if (err < 0) {
 			rcu_read_unlock();
 			drop_reason = SKB_DROP_REASON_DEV_HDR;
 			goto err_kfree;

drivers/net/tap.c static int tap_get_user_xdp(struct tap_queue *q, struct xdp_buff *xdp) · line 1090

Quoted 8 lines

drivers/net/tap.c +1 -1

@@ -1084,7 +1084,7 @@ static int tap_get_user_xdp(struct tap_queue *q, struct xdp_buff *xdp)
 
 	if (vnet_hdr_len) {
 		err = tun_vnet_hdr_to_skb(q->flags, skb, gso);
-		if (err) {
+		if (err < 0) {
 			rcu_read_unlock();
 			goto err_kfree;
 		}

similar to tun, should tap count this event?

Quoted 11 lines

drivers/net/tun.c struct tun_struct { · line 213

Quoted 9 lines

drivers/net/tun.c +1 -0

index 242899f7fd07..19cb443c89b0 100644
@@ -210,6 +210,7 @@ struct tun_struct {
 	u32 flow_count;
 	u32 rx_batched;
 	atomic_long_t rx_frame_errors;
+	atomic64_t rx_csum_sw;

is a normal (and cheaper on 32b platforms) atomic_t or atomic_long_t sufficient?

well it can overflow easily. i don't know that we care that a slow path will be even a bit slower on these ancient platforms. correct reporting seems more important.

Quoted 92 lines

Quoted 6 lines

drivers/net/tun.c +0 -0

 	struct bpf_prog __rcu *xdp_prog;
 	struct tun_prog __rcu *steering_prog;
 	struct tun_prog __rcu *filter_prog;

@@ -1935,11 +1936,16 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
 		break;

drivers/net/tun.c static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile, · line 1945

Quoted 10 lines

drivers/net/tun.c +3 -1

 	}
 
-	if (tun_vnet_hdr_tnl_to_skb(tun->flags, features, skb, &hdr)) {
+	err = tun_vnet_hdr_tnl_to_skb(tun->flags, features, skb, &hdr);
+	if (err < 0) {
 		atomic_long_inc(&tun->rx_frame_errors);
 		err = -EINVAL;
 		goto free_skb;
 	}
+	if (err > 0) {

} else if (err > 0) {

Quoted 9 lines

drivers/net/tun.c +3 -0

+		atomic64_inc(&tun->rx_csum_sw);
+		err = 0;
+	}
 
 	if ((tun->flags & TUN_TYPE_MASK) == IFF_TAP)
 		skb->protocol = eth_type_trans(skb, tun->dev);

@@ -2605,12 +2611,15 @@ static int tun_xdp_one(struct tun_struct *tun,
 	tnl_hdr = (struct virtio_net_hdr_v1_hash_tunnel *)gso;
 	skb->dev = tun->dev;

drivers/net/tun.c static int tun_xdp_one(struct tun_struct *tun, · line 2621

Quoted 10 lines

drivers/net/tun.c +3 -1

 	skb_set_network_header(skb, ETH_HLEN);
-	if (tun_vnet_hdr_tnl_to_skb(tun->flags, features, skb, tnl_hdr)) {
+	ret = tun_vnet_hdr_tnl_to_skb(tun->flags, features, skb, tnl_hdr);
+	if (ret < 0) {
 		atomic_long_inc(&tun->rx_frame_errors);
 		kfree_skb(skb);
 		ret = -EINVAL;
 		goto out;
 	}
+	if (ret > 0)

same

drivers/net/tun.c static int tun_xdp_one(struct tun_struct *tun, · line 2622

Quoted 1 line

drivers/net/tun.c +1 -0

+		atomic64_inc(&tun->rx_csum_sw);

maybe reset ret = 0 in all these branches

Quoted 47 lines

drivers/net/tun.c +28 -0

 	skb->protocol = eth_type_trans(skb, tun->dev);
 	skb_reset_network_header(skb);

@@ -3739,7 +3748,35 @@ static void tun_get_channels(struct net_device *dev,
 	channels->max_combined = tun->flags & IFF_MULTI_QUEUE ? MAX_TAP_QUEUES : 1;
 }
 
+#define TUN_STATS_LEN	1
+static const char tun_ethtool_strings[TUN_STATS_LEN][ETH_GSTRING_LEN] = {
+	"rx_csum_sw",
+};
+
+static int tun_get_sset_count(struct net_device *dev, int sset)
+{
+	return sset == ETH_SS_STATS ? TUN_STATS_LEN : -EOPNOTSUPP;
+}
+
+static void tun_get_strings(struct net_device *dev, u32 stringset, u8 *data)
+{
+	if (stringset == ETH_SS_STATS)
+		memcpy(data, tun_ethtool_strings,
+		       sizeof(tun_ethtool_strings));
+}
+
+static void tun_get_ethtool_stats(struct net_device *dev,
+				  struct ethtool_stats *stats, u64 *data)
+{
+	struct tun_struct *tun = netdev_priv(dev);
+
+	data[0] = atomic64_read(&tun->rx_csum_sw);
+}
+
 static const struct ethtool_ops tun_ethtool_ops = {
+	.get_sset_count		= tun_get_sset_count,
+	.get_strings		= tun_get_strings,
+	.get_ethtool_stats	= tun_get_ethtool_stats,
 	.supported_coalesce_params = ETHTOOL_COALESCE_RX_MAX_FRAMES,
 	.get_drvinfo	= tun_get_drvinfo,
 	.get_msglevel	= tun_get_msglevel,

drivers/net/virtio_net.c +0 -0

index daab43ac92ce..2e6fd81df9c4 100644
@@ -2519,7 +2519,7 @@ static void virtnet_receive_done(struct virtnet_info *vi, struct receive_queue *
 	skb_set_network_header(skb, ETH_HLEN);
 	if (virtio_net_hdr_tnl_to_skb(skb, &hdr->tnl_hdr, vi->rx_tnl,
 				      vi->rx_tnl_csum,

Quoted 8 lines

drivers/net/virtio_net.c static void virtnet_receive_done(struct virtnet_info *vi, struct receive_queue * · line 2525

Quoted 5 lines

drivers/net/virtio_net.c +1 -1

-				      virtio_is_little_endian(vi->vdev))) {
+				      virtio_is_little_endian(vi->vdev)) < 0) {
 		net_warn_ratelimited("%s: bad gso: type: %x, size: %u, flags %x tunnel %d tnl csum %d\n",
 				     dev->name, hdr->hdr.gso_type,
 				     hdr->hdr.gso_size, hdr->hdr.flags,

should virtio_net count these too? Or do we assume that devices are more trustworthy, so unlikely to cause this issue?

With memory encryption, from an untrusted host?
i can add that.

Quoted 27 lines

Quoted 26 lines

net/packet/af_packet.c +2 -2

index 52ae3d875e97..e6aec9ecfa99 100644
@@ -2913,7 +2913,7 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
 		}
 
 		if (has_vnet_hdr) {
-			if (virtio_net_hdr_to_skb(skb, &vnet_hdr, vio_le())) {
+			if (virtio_net_hdr_to_skb(skb, &vnet_hdr, vio_le()) < 0) {
 				tp_len = -EINVAL;
 				goto tpacket_error;
 			}

@@ -3136,7 +3136,7 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
 
 	if (vnet_hdr_sz) {
 		err = virtio_net_hdr_to_skb(skb, &vnet_hdr, vio_le());
-		if (err)
+		if (err < 0)
 			goto out_free;
 		len += vnet_hdr_sz;
 	}

Signature · 1 line

MST

Quoted 61 lines · Michael S. Tsirkin

Quoted 48 lines · Willem de Bruijn

Quoted 36 lines · Michael S. Tsirkin

The following patch will add ingress resolution of uncommon virtio net hdr checksum metadata (software checksum completion at ingress) which adds performance overhead to the packet processing.

We should make it visible, so a hypothetical workload relying on uncommon metadata can be distinguished from none at all. This way issues in the parsing will be reported and not quietly pay the host-CPU checksum overhead instead of offload.

Flip every call to virtio_net_hdr_to_skb to test for negative errno only, and have tun count these events per device as rx_csum_sw, exposed via ethtool -S.

Assisted-by LLM

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 drivers/net/tap.c        |  4 ++--
 drivers/net/tun.c        | 41 ++++++++++++++++++++++++++++++++++++++--
 drivers/net/virtio_net.c |  2 +-
 net/packet/af_packet.c   |  4 ++--
 4 files changed, 44 insertions(+), 7 deletions(-)

drivers/net/tap.c +1 -1

index ff67d99deb39..73e883394446 100644
@@ -649,7 +649,7 @@ static ssize_t tap_get_user(struct tap_queue *q, void *msg_control,
 
 	if (vnet_hdr_len) {
 		err = tun_vnet_hdr_to_skb(q->flags, skb, &vnet_hdr);
-		if (err) {
+		if (err < 0) {
 			rcu_read_unlock();
 			drop_reason = SKB_DROP_REASON_DEV_HDR;
 			goto err_kfree;

drivers/net/tap.c static int tap_get_user_xdp(struct tap_queue *q, struct xdp_buff *xdp) · line 1090

Quoted 8 lines

drivers/net/tap.c +1 -1

@@ -1084,7 +1084,7 @@ static int tap_get_user_xdp(struct tap_queue *q, struct xdp_buff *xdp)
 
 	if (vnet_hdr_len) {
 		err = tun_vnet_hdr_to_skb(q->flags, skb, gso);
-		if (err) {
+		if (err < 0) {
 			rcu_read_unlock();
 			goto err_kfree;
 		}

similar to tun, should tap count this event?

Quoted 11 lines

drivers/net/tun.c struct tun_struct { · line 213

Quoted 9 lines

drivers/net/tun.c +1 -0

index 242899f7fd07..19cb443c89b0 100644
@@ -210,6 +210,7 @@ struct tun_struct {
 	u32 flow_count;
 	u32 rx_batched;
 	atomic_long_t rx_frame_errors;
+	atomic64_t rx_csum_sw;

is a normal (and cheaper on 32b platforms) atomic_t or atomic_long_t sufficient?

Quoted 3 lines

well it can overflow easily. i don't know that we care that a slow path will be even a bit slower on these ancient platforms. correct reporting seems more important.

Ok. I would hope that monitoring catches this before it hits 4B, but agreed that as slow path it doesn't matter.

Quoted 101 lines

Quoted 100 lines

Quoted 6 lines

drivers/net/tun.c +0 -0

 	struct bpf_prog __rcu *xdp_prog;
 	struct tun_prog __rcu *steering_prog;
 	struct tun_prog __rcu *filter_prog;

@@ -1935,11 +1936,16 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
 		break;

drivers/net/tun.c static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile, · line 1945

Quoted 10 lines

drivers/net/tun.c +3 -1

 	}
 
-	if (tun_vnet_hdr_tnl_to_skb(tun->flags, features, skb, &hdr)) {
+	err = tun_vnet_hdr_tnl_to_skb(tun->flags, features, skb, &hdr);
+	if (err < 0) {
 		atomic_long_inc(&tun->rx_frame_errors);
 		err = -EINVAL;
 		goto free_skb;
 	}
+	if (err > 0) {

} else if (err > 0) {

Quoted 9 lines

drivers/net/tun.c +3 -0

+		atomic64_inc(&tun->rx_csum_sw);
+		err = 0;
+	}
 
 	if ((tun->flags & TUN_TYPE_MASK) == IFF_TAP)
 		skb->protocol = eth_type_trans(skb, tun->dev);

@@ -2605,12 +2611,15 @@ static int tun_xdp_one(struct tun_struct *tun,
 	tnl_hdr = (struct virtio_net_hdr_v1_hash_tunnel *)gso;
 	skb->dev = tun->dev;

drivers/net/tun.c static int tun_xdp_one(struct tun_struct *tun, · line 2621

Quoted 10 lines

drivers/net/tun.c +3 -1

 	skb_set_network_header(skb, ETH_HLEN);
-	if (tun_vnet_hdr_tnl_to_skb(tun->flags, features, skb, tnl_hdr)) {
+	ret = tun_vnet_hdr_tnl_to_skb(tun->flags, features, skb, tnl_hdr);
+	if (ret < 0) {
 		atomic_long_inc(&tun->rx_frame_errors);
 		kfree_skb(skb);
 		ret = -EINVAL;
 		goto out;
 	}
+	if (ret > 0)

same

drivers/net/tun.c static int tun_xdp_one(struct tun_struct *tun, · line 2622

Quoted 1 line

drivers/net/tun.c +1 -0

+		atomic64_inc(&tun->rx_csum_sw);

maybe reset ret = 0 in all these branches

Quoted 44 lines

drivers/net/tun.c +28 -0

 	skb->protocol = eth_type_trans(skb, tun->dev);
 	skb_reset_network_header(skb);

@@ -3739,7 +3748,35 @@ static void tun_get_channels(struct net_device *dev,
 	channels->max_combined = tun->flags & IFF_MULTI_QUEUE ? MAX_TAP_QUEUES : 1;
 }
 
+#define TUN_STATS_LEN	1
+static const char tun_ethtool_strings[TUN_STATS_LEN][ETH_GSTRING_LEN] = {
+	"rx_csum_sw",
+};
+
+static int tun_get_sset_count(struct net_device *dev, int sset)
+{
+	return sset == ETH_SS_STATS ? TUN_STATS_LEN : -EOPNOTSUPP;
+}
+
+static void tun_get_strings(struct net_device *dev, u32 stringset, u8 *data)
+{
+	if (stringset == ETH_SS_STATS)
+		memcpy(data, tun_ethtool_strings,
+		       sizeof(tun_ethtool_strings));
+}
+
+static void tun_get_ethtool_stats(struct net_device *dev,
+				  struct ethtool_stats *stats, u64 *data)
+{
+	struct tun_struct *tun = netdev_priv(dev);
+
+	data[0] = atomic64_read(&tun->rx_csum_sw);
+}
+
 static const struct ethtool_ops tun_ethtool_ops = {
+	.get_sset_count		= tun_get_sset_count,
+	.get_strings		= tun_get_strings,
+	.get_ethtool_stats	= tun_get_ethtool_stats,
 	.supported_coalesce_params = ETHTOOL_COALESCE_RX_MAX_FRAMES,
 	.get_drvinfo	= tun_get_drvinfo,
 	.get_msglevel	= tun_get_msglevel,

drivers/net/virtio_net.c +0 -0

index daab43ac92ce..2e6fd81df9c4 100644

drivers/net/virtio_net.c static void virtnet_receive_done(struct virtnet_info *vi, struct receive_queue * · line 2525

Quoted 8 lines

drivers/net/virtio_net.c +1 -1

@@ -2519,7 +2519,7 @@ static void virtnet_receive_done(struct virtnet_info *vi, struct receive_queue *
 	skb_set_network_header(skb, ETH_HLEN);
 	if (virtio_net_hdr_tnl_to_skb(skb, &hdr->tnl_hdr, vi->rx_tnl,
 				      vi->rx_tnl_csum,
-				      virtio_is_little_endian(vi->vdev))) {
+				      virtio_is_little_endian(vi->vdev)) < 0) {
 		net_warn_ratelimited("%s: bad gso: type: %x, size: %u, flags %x tunnel %d tnl csum %d\n",
 				     dev->name, hdr->hdr.gso_type,
 				     hdr->hdr.gso_size, hdr->hdr.flags,

should virtio_net count these too? Or do we assume that devices are more trustworthy, so unlikely to cause this issue?

Quoted 2 lines

With memory encryption, from an untrusted host?
i can add that.

I honestly don't know if it's worth the effort. Just curious whether you had purposely skipped this and tap.

Quoted 28 lines

Quoted 27 lines

Quoted 26 lines

net/packet/af_packet.c +2 -2

index 52ae3d875e97..e6aec9ecfa99 100644
@@ -2913,7 +2913,7 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
 		}
 
 		if (has_vnet_hdr) {
-			if (virtio_net_hdr_to_skb(skb, &vnet_hdr, vio_le())) {
+			if (virtio_net_hdr_to_skb(skb, &vnet_hdr, vio_le()) < 0) {
 				tp_len = -EINVAL;
 				goto tpacket_error;
 			}

@@ -3136,7 +3136,7 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
 
 	if (vnet_hdr_sz) {
 		err = virtio_net_hdr_to_skb(skb, &vnet_hdr, vio_le());
-		if (err)
+		if (err < 0)
 			goto out_free;
 		len += vnet_hdr_sz;
 	}

Signature · 1 line

MST

virtio_net_hdr_to_skb() refuses packets whose checksum metadata it cannot make sense of, but the vector raw transport ignored its return value: rejected packets went on into the stack with their metadata unvalidated.

Propagate the failure.

Note that a follow-up patch will modify virtio_net_hdr_to_skb() to return a negative value on error and a non-negative value on success. This is why we don't just propagate its return value directly.

Assisted-by LLM

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 arch/um/drivers/vector_transports.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

arch/um/drivers/vector_transports.c +0 -0

index e1fb2a76fdff..8b7548458663 100644

arch/um/drivers/vector_transports.c +0 -0

 	return 0;
 }
 

Signature · 1 line

MST

As reported first by Paulos Yibelo <[email protected]> (after which others, credited below, found more ways the bug can affect users) a virtio net header can place CHECKSUM_PARTIAL at any in-packet offset. If that state is retained, later checksum completion can modify the packet after protocol processing has made decisions from it.

Let's fix that: let's validate csum_start and csum_offset and only keep CHECKSUM_PARTIAL when csum_start and csum_offset match a parsed TCP or UDP header, while handling VLAN headers, IPv4 options and IPv6 extension headers. Complete other non-GSO checksums immediately, before any protocol processing runs.

We can't easily "complete the checksum" for GSO we don't recognize - GSO assumes knowledge of the protocol. We could segment ignoring the checksum then do whatever csum_start and csum_offset say, but that seems like an overkill for something that shouldn't happen.

There's some risk here:
- non-gso packets which can't be parsed get checksum calculated in software, adding performance overhead
- gso packets which can't be parsed get dropped

To mitigate the risks, the following patch will add a sysctl knob to bypass the resolution. This is reasonable if the setup is such that there is no protocol processing at all.

UDP tunnel headers get the same handling on the inner header.

Packets without VIRTIO_NET_HDR_F_NEEDS_CSUM are unaffected.

Assisted-by LLM

Reported-by Paulos Yibelo <[email protected]>

Reported-by Chris Arges <[email protected]>

Tested-by Chris Arges <[email protected]>

Fixes f43798c27684 ("tun: Allow GSO using virtio_net_hdr")

Suggested-by Willem de Bruijn <[email protected]>

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 include/linux/virtio_net.h | 272 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 268 insertions(+), 4 deletions(-)

include/linux/virtio_net.h +46 -0

index 013c6c7e2dea..97b554ef7123 100644
@@ -5,7 +5,11 @@
 #include <linux/if_vlan.h>
 #include <linux/ip.h>
 #include <linux/ipv6.h>
+#include <linux/netdevice.h>
 #include <linux/udp.h>
+#include <net/gso.h>
+#include <net/ip.h>
+#include <net/ipv6.h>
 #include <net/tcp.h>
 #include <uapi/linux/tcp.h>
 #include <uapi/linux/virtio_net.h>

@@ -48,6 +52,198 @@ static inline int virtio_net_hdr_set_proto(struct sk_buff *skb,
 	return 0;
 }
 
+/* TODO: nothing virtio specific here, move to ip.h */
+
+/*
+ * Pull and minimally validate the IPv4 header at @nhoff.
+ * Returns the header and stores the transport header offset in
+ * @thoff, or NULL if truncated or malformed.  Only the fixed header
+ * is guaranteed linear; callers that need the options or the
+ * transport header must pull further.
+ */
+static inline struct iphdr *virtio_net_ip4_parse(struct sk_buff *skb,
+						  unsigned int nhoff,
+						  unsigned int *thoff)
+{
+	struct iphdr *iph;
+
+	if (!pskb_may_pull(skb, nhoff + sizeof(struct iphdr)))
+		return NULL;
+
+	iph = (struct iphdr *)(skb->data + nhoff);
+	if (iph->version != 4 ||
+	    iph->ihl < sizeof(struct iphdr) / sizeof(u32))
+		return NULL;
+
+	*thoff = nhoff + iph->ihl * sizeof(u32);
+	return iph;
+}
+
+/* Where does the L3 header start, and what protocol is it?
+ * Handles Ethernet and VLAN headers; other devices use the
+ * network header offset the caller set up.
+ */
+static inline bool
+virtio_net_l3_start(struct sk_buff *skb, int *nhoff, __be16 *proto)
+{
+	/* skb->dev is only guaranteed to be set since 44378d02c5aa
+	 * ("net: always dissect GSO packets in
+	 * __virtio_net_hdr_to_skb()"), do not crash when it is not.
+	 */
+	if (WARN_ON_ONCE(!skb->dev))
+		return false;
+
+	if (skb->dev->type == ARPHRD_ETHER) {

include/linux/virtio_net.h +7 -0

+		*nhoff = skb_network_offset(skb);
+		/* An unset network header shows up as an offset
+		 * outside of the packet.
+		 */
+		if (*nhoff < 0 || (unsigned int)*nhoff >= skb->len)
+			return false;
+		*proto = skb->protocol;

include/linux/virtio_net.h +39 -0

+			if (version == 4)
+				*proto = htons(ETH_P_IP);
+			else if (version == 6)
+				*proto = htons(ETH_P_IPV6);
+		}
+	}
+	return true;
+}
+
+/* Walk the L3 header at @nhoff: where does its transport header
+ * start, and which protocol is it?  Returns false if the header
+ * is truncated, malformed or fragmented.
+ */
+static inline bool
+virtio_net_l4_start(struct sk_buff *skb, unsigned int nhoff, __be16 proto,
+		    unsigned int *thoff, int *ip_proto)
+{
+	if (proto == htons(ETH_P_IP)) {
+		const struct iphdr *iph = virtio_net_ip4_parse(skb, nhoff, thoff);
+
+		if (!iph || ip_is_fragment(iph))
+			return false;
+		*ip_proto = iph->protocol;
+		return true;
+	}
+
+	if (proto == htons(ETH_P_IPV6)) {
+		const struct ipv6hdr *iph;
+		int flags = 0;
+
+		if (!pskb_may_pull(skb, nhoff + sizeof(*iph)))
+			return false;
+		iph = (const void *)(skb->data + nhoff);
+		if (iph->version != 6)
+			return false;
+		*thoff = nhoff + sizeof(*iph);
+		*ip_proto = iph->nexthdr;
+		if (*ip_proto != IPPROTO_TCP && *ip_proto != IPPROTO_UDP) {
+			*thoff = nhoff;

include/linux/virtio_net.h +90 -1

+
+/* Does checksum metadata match a known TCP or UDP header? */
+static inline bool
+virtio_net_csum_partial_ok_at_offset(struct sk_buff *skb, u8 hdr_gso_type,
+				     unsigned int nhoff, __be16 proto)
+{
+	unsigned int thoff;
+	int ip_proto;
+
+	if (!virtio_net_l4_start(skb, nhoff, proto, &thoff, &ip_proto))
+		return false;
+
+	if (skb_checksum_start_offset(skb) != thoff)
+		return false;
+
+	switch (ip_proto) {
+	case IPPROTO_TCP:
+		if (skb->csum_offset != offsetof(struct tcphdr, check) ||
+		    !pskb_may_pull(skb, thoff + sizeof(struct tcphdr)))
+			return false;
+		break;
+	case IPPROTO_UDP:
+		if (skb->csum_offset != offsetof(struct udphdr, check) ||
+		    !pskb_may_pull(skb, thoff + sizeof(struct udphdr)))
+			return false;
+		break;
+	default:
+		return false;
+	}
+
+	switch (hdr_gso_type & ~VIRTIO_NET_HDR_GSO_ECN) {
+	case VIRTIO_NET_HDR_GSO_NONE:
+		return true;
+	case VIRTIO_NET_HDR_GSO_TCPV4:
+		return proto == htons(ETH_P_IP) && ip_proto == IPPROTO_TCP;
+	case VIRTIO_NET_HDR_GSO_TCPV6:
+		return proto == htons(ETH_P_IPV6) && ip_proto == IPPROTO_TCP;
+	case VIRTIO_NET_HDR_GSO_UDP:
+	case VIRTIO_NET_HDR_GSO_UDP_L4:
+		return ip_proto == IPPROTO_UDP;
+	default:
+		return false;
+	}
+}
+
+static inline bool
+virtio_net_csum_partial_ok(struct sk_buff *skb, u8 hdr_gso_type)
+{
+	int nhoff;
+	__be16 proto;
+
+	if (!virtio_net_l3_start(skb, &nhoff, &proto))
+		return false;
+
+	return virtio_net_csum_partial_ok_at_offset(skb, hdr_gso_type, nhoff,
+						   proto);
+}
+
+/*
+ * Handle a packet whose checksum metadata does not match the packet:
+ * GSO packets are dropped, as segmentation needs one checksum per
+ * segment; others are completed in software.
+ *
+ * Return 1 if the checksum was completed in software, else a negative
+ * errno.
+ */
+static inline int virtio_net_resolve_csum_partial_mismatch(struct sk_buff *skb)
+{
+	if (skb_is_gso(skb))
+		return -EINVAL;
+
+	return skb_checksum_help(skb) ?: 1;
+}
+
 static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,
 					  const struct virtio_net_hdr *hdr,
 					  bool little_endian, u8 hdr_gso_type)

@@ -194,11 +390,36 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,
 	return 0;
 }
 
+/*
+ * Return value:
+ * 0 - success
+ * >0 - checksum was completed in software (slow path)
+ * <0 - failure
+ */
 static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
 					const struct virtio_net_hdr *hdr,
 					bool little_endian)
 {
-	return __virtio_net_hdr_to_skb(skb, hdr, little_endian, hdr->gso_type);
+	typeof(skb->transport_header) old_transport = skb->transport_header;
+	int ret = __virtio_net_hdr_to_skb(skb, hdr, little_endian,
+					  hdr->gso_type);
+
+	if (ret < 0)
+		return ret;
+
+	if (skb->ip_summed != CHECKSUM_PARTIAL ||
+	    virtio_net_csum_partial_ok(skb, hdr->gso_type))
+		return 0;

include/linux/virtio_net.h +34 -1

 }
 
 /* This function must be called after virtio_net_hdr_from_skb(). */

@@ -282,6 +503,35 @@ static inline unsigned int virtio_l3min(bool is_ipv6)
 	return is_ipv6 ? sizeof(struct ipv6hdr) : sizeof(struct iphdr);
 }
 
+/* Do the tunnel offsets match the outer headers in the frame?
+ * The L3 header the tunnel claims must be the one the frame
+ * carries, with UDP following it at @outer_th.
+ */
+static inline bool
+virtio_net_tnl_offsets_ok(struct sk_buff *skb, bool outer_isv6,
+			  unsigned int outer_th)
+{
+	int nhoff;
+	__be16 proto;
+	unsigned int thoff;
+	int ip_proto;
+
+	if (!virtio_net_l3_start(skb, &nhoff, &proto))
+		return false;
+	if (outer_isv6 != (proto == htons(ETH_P_IPV6)))
+		return false;
+	if (!virtio_net_l4_start(skb, nhoff, proto, &thoff, &ip_proto))
+		return false;
+
+	return ip_proto == IPPROTO_UDP && outer_th == thoff;
+}
+
+/*
+ * Return value:
+ * 0 - success
+ * >0 - checksum was completed in software (slow path)
+ * <0 - failure
+ */
 static inline int
 virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
 			  const struct virtio_net_hdr_v1_hash_tunnel *vhdr,

@@ -293,6 +543,7 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
 	unsigned int inner_nh, outer_th, inner_th;
 	unsigned int inner_l3min, outer_l3min;
 	u8 gso_inner_type, gso_tunnel_type;
+	__be16 inner_proto;
 	bool outer_isv6, inner_isv6;
 	int ret;
 

@@ -335,9 +586,12 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
 	/* Let the basic parsing deal with plain GSO features. */
 	ret = __virtio_net_hdr_to_skb(skb, hdr, true,
 				      hdr->gso_type & ~gso_tunnel_type);
-	if (ret)
+	if (ret < 0)
 		return ret;
 
+	if (!virtio_net_tnl_offsets_ok(skb, outer_isv6, outer_th))
+		return -EINVAL;
+
 	/* In case of USO, the inner protocol is still unknown and
 	 * `inner_isv6` is just a guess, additional parsing is needed.
 	 * The previous validation ensures that accessing an ipv4 inner

include/linux/virtio_net.h +1 -1

 	if (hdr->flags & VIRTIO_NET_HDR_F_UDP_TUNNEL_CSUM) {
 		if (!tnl_csum_negotiated)
 			return -EINVAL;

@@ -368,7 +622,17 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
 	skb->inner_mac_header = inner_nh + skb_headroom(skb);
 	skb->transport_header = outer_th + skb_headroom(skb);
 	skb->encapsulation = 1;
-	return 0;
+

include/linux/virtio_net.h +0 -0

 }
 
 /* Checksum-related fields validation for the driver */

Signature · 1 line

MST

Quoted 35 lines

include/linux/virtio_net.h +35 -0

+/* Walk the L3 header at @nhoff: where does its transport header
+ * start, and which protocol is it?  Returns false if the header
+ * is truncated, malformed or fragmented.
+ */
+static inline bool
+virtio_net_l4_start(struct sk_buff *skb, unsigned int nhoff, __be16 proto,
+		    unsigned int *thoff, int *ip_proto)
+{
+	if (proto == htons(ETH_P_IP)) {
+		const struct iphdr *iph = virtio_net_ip4_parse(skb, nhoff, thoff);
+
+		if (!iph || ip_is_fragment(iph))
+			return false;
+		*ip_proto = iph->protocol;
+		return true;
+	}
+
+	if (proto == htons(ETH_P_IPV6)) {
+		const struct ipv6hdr *iph;
+		int flags = 0;
+
+		if (!pskb_may_pull(skb, nhoff + sizeof(*iph)))
+			return false;
+		iph = (const void *)(skb->data + nhoff);
+		if (iph->version != 6)
+			return false;
+		*thoff = nhoff + sizeof(*iph);
+		*ip_proto = iph->nexthdr;
+		if (*ip_proto != IPPROTO_TCP && *ip_proto != IPPROTO_UDP) {
+			*thoff = nhoff;
+			*ip_proto = ipv6_find_hdr(skb, thoff, -1, NULL,
+						  &flags);
+			if (*ip_proto < 0 || flags & IP6_FH_F_FRAG)
+				return false;
+		}

include/linux/virtio_net.h static inline int virtio_net_hdr_set_proto(struct sk_buff *skb, · line 148

Quoted 5 lines

include/linux/virtio_net.h +5 -0

+		return true;
+	}
+
+	return false;
+}

for all this parsing: could we use flow dissection, like the !VIRTIO_NET_HDR_F_NEEDS_CSUM case?

Quoted 35 lines · Willem de Bruijn

Quoted 35 lines

include/linux/virtio_net.h +35 -0

+/* Walk the L3 header at @nhoff: where does its transport header
+ * start, and which protocol is it?  Returns false if the header
+ * is truncated, malformed or fragmented.
+ */
+static inline bool
+virtio_net_l4_start(struct sk_buff *skb, unsigned int nhoff, __be16 proto,
+		    unsigned int *thoff, int *ip_proto)
+{
+	if (proto == htons(ETH_P_IP)) {
+		const struct iphdr *iph = virtio_net_ip4_parse(skb, nhoff, thoff);
+
+		if (!iph || ip_is_fragment(iph))
+			return false;
+		*ip_proto = iph->protocol;
+		return true;
+	}
+
+	if (proto == htons(ETH_P_IPV6)) {
+		const struct ipv6hdr *iph;
+		int flags = 0;
+
+		if (!pskb_may_pull(skb, nhoff + sizeof(*iph)))
+			return false;
+		iph = (const void *)(skb->data + nhoff);
+		if (iph->version != 6)
+			return false;
+		*thoff = nhoff + sizeof(*iph);
+		*ip_proto = iph->nexthdr;
+		if (*ip_proto != IPPROTO_TCP && *ip_proto != IPPROTO_UDP) {
+			*thoff = nhoff;
+			*ip_proto = ipv6_find_hdr(skb, thoff, -1, NULL,
+						  &flags);
+			if (*ip_proto < 0 || flags & IP6_FH_F_FRAG)
+				return false;
+		}

Quoted 8 lines

include/linux/virtio_net.h static inline int virtio_net_hdr_set_proto(struct sk_buff *skb, · line 148

Quoted 5 lines

include/linux/virtio_net.h +5 -0

+		return true;
+	}
+
+	return false;
+}

for all this parsing: could we use flow dissection, like the !VIRTIO_NET_HDR_F_NEEDS_CSUM case?

Hmm I don't see how. That one trusts the caller's protocol and network header. We could parse L2 ourselves and use the dissector for L3+. But it stops at whatever it does not know, and every miss would turn into a drop or a software checksum for a packet the stack delivers fine. Our parser deliberately covers everything the stack can deliver.

Quoted 45 lines · Michael S. Tsirkin

Quoted 35 lines · Willem de Bruijn

Quoted 35 lines

include/linux/virtio_net.h +35 -0

+/* Walk the L3 header at @nhoff: where does its transport header
+ * start, and which protocol is it?  Returns false if the header
+ * is truncated, malformed or fragmented.
+ */
+static inline bool
+virtio_net_l4_start(struct sk_buff *skb, unsigned int nhoff, __be16 proto,
+		    unsigned int *thoff, int *ip_proto)
+{
+	if (proto == htons(ETH_P_IP)) {
+		const struct iphdr *iph = virtio_net_ip4_parse(skb, nhoff, thoff);
+
+		if (!iph || ip_is_fragment(iph))
+			return false;
+		*ip_proto = iph->protocol;
+		return true;
+	}
+
+	if (proto == htons(ETH_P_IPV6)) {
+		const struct ipv6hdr *iph;
+		int flags = 0;
+
+		if (!pskb_may_pull(skb, nhoff + sizeof(*iph)))
+			return false;
+		iph = (const void *)(skb->data + nhoff);
+		if (iph->version != 6)
+			return false;
+		*thoff = nhoff + sizeof(*iph);
+		*ip_proto = iph->nexthdr;
+		if (*ip_proto != IPPROTO_TCP && *ip_proto != IPPROTO_UDP) {
+			*thoff = nhoff;
+			*ip_proto = ipv6_find_hdr(skb, thoff, -1, NULL,
+						  &flags);
+			if (*ip_proto < 0 || flags & IP6_FH_F_FRAG)
+				return false;
+		}

Quoted 8 lines

include/linux/virtio_net.h static inline int virtio_net_hdr_set_proto(struct sk_buff *skb, · line 148

Quoted 5 lines

include/linux/virtio_net.h +5 -0

+		return true;
+	}
+
+	return false;
+}

for all this parsing: could we use flow dissection, like the !VIRTIO_NET_HDR_F_NEEDS_CSUM case?

Quoted 5 lines

Hmm I don't see how. That one trusts the caller's protocol and network header. We could parse L2 ourselves and use the dissector for L3+. But it stops at whatever it does not know, and every miss would turn into a drop or a software checksum for a packet the stack delivers fine. Our parser deliberately covers everything the stack can deliver.

You're right, the protocol and network offset need to be parsed manually. After that it could be passed to the flow dissector.

Totally untested:

    static inline bool virtio_net_setup_l3_input(struct sk_buff *skb, int *nhoff, __be16 *proto)
    {
        const struct net_device *dev = skb->dev;

        /* Devices with Ethernet framing: ARPHRD_ETHER, loopback, macvlan, etc. */
        if (dev->header_ops == &eth_header_ops || dev->type == ARPHRD_ETHER) {
            if (!pskb_may_pull(skb, ETH_HLEN))
                return false;
            *nhoff = ETH_HLEN;
            *proto = eth_hdr(skb)->h_proto;
        } else if (dev->type == ARPHRD_NONE) {
            /* Raw L3 devices (e.g. tun in IFF_TUN mode) */
            *nhoff = 0;
            *proto = skb->protocol;
            if (!*proto) {
                if (!pskb_may_pull(skb, 1))
                    return false;
                if ((skb->data[0] >> 4) == 4)
                    *proto = htons(ETH_P_IP);
                else if ((skb->data[0] >> 4) == 6)
                    *proto = htons(ETH_P_IPV6);
                else
                    return false;
            }
        } else {
            /* Fallback for other netdevs using caller's network header */
            *nhoff = skb_network_offset(skb);
            if (*nhoff < 0 || (unsigned int)*nhoff >= skb->len)
                return false;
            *proto = skb->protocol;
        }

        return true;
    }

    static inline bool virtio_net_csum_partial_ok(struct sk_buff *skb, u8 hdr_gso_type)
    {
        struct flow_keys_basic keys;
        __be16 orig_proto = skb->protocol;
        u16 orig_net_hdr = skb->network_header;
        __be16 proto;
        int nhoff;
        u16 csum_offset;
        size_t min_thlen;
  
        if (!virtio_net_setup_l3_input(skb, &nhoff, &proto))
            return false;
  
        skb_set_network_header(skb, nhoff);
        skb->protocol = proto;
  
        if (!skb_flow_dissect_flow_keys_basic(dev_net(skb->dev), skb, &keys,
                              NULL, 0, 0, 0, 0))
            goto restore_fail;
  
        /* Disallow fragments: partial checksums cannot be retained across frags */
        if (keys.control.flags & FLOW_DIS_IS_FRAGMENT)
            goto restore_fail;
  
        /* Validate protocol matching */
        switch (keys.basic.ip_proto) {
        case IPPROTO_TCP:
            if (!virtio_net_hdr_match_proto(keys.basic.n_proto, hdr_gso_type))
                goto restore_fail;
            min_thlen = sizeof(struct tcphdr);
            csum_offset = offsetof(struct tcphdr, check);
            break;
        case IPPROTO_UDP:
            if (!virtio_net_hdr_match_proto(keys.basic.n_proto, hdr_gso_type))
                goto restore_fail;
            min_thlen = sizeof(struct udphdr);
            csum_offset = offsetof(struct udphdr, check);
            break;
        default:
            goto restore_fail;
        }

        /* csum_start must match the dissected transport header offset */
        if (skb_transport_offset(skb) != keys.control.thoff)
            goto restore_fail;

        /* csum_offset must point to the transport checksum field */
        if (skb->csum_offset != csum_offset)
            goto restore_fail;

        /* Ensure the transport header is linear */
        if (!pskb_may_pull(skb, keys.control.thoff + min_thlen))
            goto restore_fail;

        return true;

    restore_fail:
        skb->protocol = orig_proto;
        skb->network_header = orig_net_hdr;
        return false;
    }

Quoted 45 lines · Michael S. Tsirkin

Quoted 43 lines · Willem de Bruijn

Quoted 30 lines

include/linux/virtio_net.h +30 -0

+/* Walk the L3 header at @nhoff: where does its transport header
+ * start, and which protocol is it?  Returns false if the header
+ * is truncated, malformed or fragmented.
+ */
+static inline bool
+virtio_net_l4_start(struct sk_buff *skb, unsigned int nhoff, __be16 proto,
+		    unsigned int *thoff, int *ip_proto)
+{
+	if (proto == htons(ETH_P_IP)) {
+		const struct iphdr *iph = virtio_net_ip4_parse(skb, nhoff, thoff);
+
+		if (!iph || ip_is_fragment(iph))
+			return false;
+		*ip_proto = iph->protocol;
+		return true;
+	}
+
+	if (proto == htons(ETH_P_IPV6)) {
+		const struct ipv6hdr *iph;
+		int flags = 0;
+
+		if (!pskb_may_pull(skb, nhoff + sizeof(*iph)))
+			return false;
+		iph = (const void *)(skb->data + nhoff);
+		if (iph->version != 6)
+			return false;
+		*thoff = nhoff + sizeof(*iph);
+		*ip_proto = iph->nexthdr;
+		if (*ip_proto != IPPROTO_TCP && *ip_proto != IPPROTO_UDP) {
+			*thoff = nhoff;

include/linux/virtio_net.h static inline int virtio_net_hdr_set_proto(struct sk_buff *skb, · line 172

Quoted 10 lines

include/linux/virtio_net.h +10 -0

+			*ip_proto = ipv6_find_hdr(skb, thoff, -1, NULL,
+						  &flags);
+			if (*ip_proto < 0 || flags & IP6_FH_F_FRAG)
+				return false;
+		}
+		return true;
+	}
+
+	return false;
+}

for all this parsing: could we use flow dissection, like the !VIRTIO_NET_HDR_F_NEEDS_CSUM case?

Quoted 5 lines

Hmm I don't see how. That one trusts the caller's protocol and network header. We could parse L2 ourselves and use the dissector for L3+. But it stops at whatever it does not know, and every miss would turn into a drop or a software checksum for a packet the stack delivers fine. Our parser deliberately covers everything the stack can deliver.

You're right, the protocol and network offset need to be parsed manually. After that it could be passed to the flow dissector.

Quoted 52 lines · Willem de Bruijn

Quoted 50 lines · Michael S. Tsirkin

Quoted 35 lines · Willem de Bruijn

Quoted 35 lines

include/linux/virtio_net.h +35 -0

+/* Walk the L3 header at @nhoff: where does its transport header
+ * start, and which protocol is it?  Returns false if the header
+ * is truncated, malformed or fragmented.
+ */
+static inline bool
+virtio_net_l4_start(struct sk_buff *skb, unsigned int nhoff, __be16 proto,
+		    unsigned int *thoff, int *ip_proto)
+{
+	if (proto == htons(ETH_P_IP)) {
+		const struct iphdr *iph = virtio_net_ip4_parse(skb, nhoff, thoff);
+
+		if (!iph || ip_is_fragment(iph))
+			return false;
+		*ip_proto = iph->protocol;
+		return true;
+	}
+
+	if (proto == htons(ETH_P_IPV6)) {
+		const struct ipv6hdr *iph;
+		int flags = 0;
+
+		if (!pskb_may_pull(skb, nhoff + sizeof(*iph)))
+			return false;
+		iph = (const void *)(skb->data + nhoff);
+		if (iph->version != 6)
+			return false;
+		*thoff = nhoff + sizeof(*iph);
+		*ip_proto = iph->nexthdr;
+		if (*ip_proto != IPPROTO_TCP && *ip_proto != IPPROTO_UDP) {
+			*thoff = nhoff;
+			*ip_proto = ipv6_find_hdr(skb, thoff, -1, NULL,
+						  &flags);
+			if (*ip_proto < 0 || flags & IP6_FH_F_FRAG)
+				return false;
+		}

Quoted 8 lines

include/linux/virtio_net.h static inline int virtio_net_hdr_set_proto(struct sk_buff *skb, · line 148

Quoted 5 lines

include/linux/virtio_net.h +5 -0

+		return true;
+	}
+
+	return false;
+}

for all this parsing: could we use flow dissection, like the !VIRTIO_NET_HDR_F_NEEDS_CSUM case?

Hmm I don't see how. That one trusts the caller's protocol and network header. We could parse L2 ourselves and use the dissector for L3+. But it stops at whatever it does not know, and every miss would turn into a drop or a software checksum for a packet the stack delivers fine. Our parser deliberately covers everything the stack can deliver.

Quoted 2 lines

You're right, the protocol and network offset need to be parsed manually. After that it could be passed to the flow dissector.

I posted v4 with the custom parser, since i had it tested. Let's do cleanups on top? I spent too much time on this, to be frank.

Hmm, an interesting trick, but I don't see how this works for tunnels? We need to dissect at inner_nh, and on success we can not leave network_header there - the packet continues to outer IP processing. I fear making all this work will be more code than the parsing it replaces.

The ingress validation as implemented by the previous patch is stricter than what it replaced: GSO
packets whose checksum metadata matches no transport header are dropped, and non-GSO ones get their checksum completed in place. Such packets used to pass with the metadata unvalidated, and with no overhead.

I judge that this should not affect any deployments, but there is still some risk that it does. In case a deployment depends on them anyway, provide a per-netns switch to get the old behavior back:

echo 0 > /proc/sys/net/core/virtio_net_hdr_csum_check

The default is validating.

Assisted-by LLM

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 Documentation/admin-guide/sysctl/net.rst | 19 ++++++++
 include/linux/virtio_net.h               | 23 +++++++++
 include/net/netns/core.h                 |  1 +
 net/core/dev.c                           |  6 +++
 net/core/net_namespace.c                 |  1 +
 net/core/sysctl_net_core.c               | 60 ++++++++++++++++++++++++
 6 files changed, 110 insertions(+)

Documentation/admin-guide/sysctl/net.rst +19 -0

index e586e17fc7a5..b1a1c4714b22 100644
@@ -246,6 +246,25 @@ processes are dropped unless socket option SOF_TIMESTAMPING_OPT_TSONLY is set.
 Default: 1 (on)
 
 
+virtio_net_hdr_csum_check
+-------------------------
+
+When processing packets with a virtio net header (through tun, tap, a
+packet socket or a virtio network device) that request checksum offload
+(VIRTIO_NET_HDR_F_NEEDS_CSUM), validate the packet: complete the checksum
+in software, or drop the packet where the header does not match it.
+
+When disabled, the header is trusted not to corrupt the packet, and the
+checksum is offloaded to hardware (or not verified if it never reaches
+hardware). This is only safe if no protocol is processing the packets.
+
+The setting is per network namespace, but only writable by the host
+root: root inside a network namespace created by an unprivileged user
+cannot disable the validation.
+
+Default: 1 (on)
+
+
 wmem_default
 ------------
 

include/linux/virtio_net.h +23 -0

index 97b554ef7123..25aa95c11e31 100644
@@ -5,6 +5,7 @@
 #include <linux/if_vlan.h>
 #include <linux/ip.h>
 #include <linux/ipv6.h>
+#include <linux/jump_label.h>
 #include <linux/netdevice.h>
 #include <linux/udp.h>
 #include <net/gso.h>

@@ -78,6 +79,19 @@ static inline struct iphdr *virtio_net_ip4_parse(struct sk_buff *skb,
 	*thoff = nhoff + iph->ihl * sizeof(u32);
 	return iph;
 }
+/* Defined in net/core/dev.c, enabled while any network namespace
+ * has net.core.virtio_net_hdr_csum_check set to 0.
+ */
+DECLARE_STATIC_KEY_FALSE(virtio_net_hdr_csum_check_disabled_key);
+
+/* Failsafe: a namespace can disable the check with
+ * net.core.virtio_net_hdr_csum_check.
+ */
+static inline bool virtio_net_hdr_csum_check_disabled(struct sk_buff *skb)
+{
+	return static_branch_unlikely(&virtio_net_hdr_csum_check_disabled_key) &&
+	       !READ_ONCE(dev_net(skb->dev)->core.sysctl_virtio_net_hdr_csum_check);
+}
 
 /* Where does the L3 header start, and what protocol is it?
  * Handles Ethernet and VLAN headers; other devices use the

@@ -179,6 +193,12 @@ virtio_net_csum_partial_ok_at_offset(struct sk_buff *skb, u8 hdr_gso_type,
 	unsigned int thoff;
 	int ip_proto;
 
+	/* Failsafe: a namespace opted out of the validation,
+	 * trust the checksum metadata as before.
+	 */
+	if (virtio_net_hdr_csum_check_disabled(skb))
+		return true;
+
 	if (!virtio_net_l4_start(skb, nhoff, proto, &thoff, &ip_proto))
 		return false;
 

@@ -516,6 +536,9 @@ virtio_net_tnl_offsets_ok(struct sk_buff *skb, bool outer_isv6,
 	unsigned int thoff;
 	int ip_proto;
 
+	if (virtio_net_hdr_csum_check_disabled(skb))
+		return true;
+
 	if (!virtio_net_l3_start(skb, &nhoff, &proto))
 		return false;
 	if (outer_isv6 != (proto == htons(ETH_P_IPV6)))

include/net/netns/core.h +1 -0

index 9ef3d70e5e9c..ae8110772c97 100644
@@ -18,6 +18,7 @@ struct netns_core {
 	u8	sysctl_txrehash;
 	u8	sysctl_tstamp_allow_data;
 	u8	sysctl_bypass_prot_mem;
+	u8	sysctl_virtio_net_hdr_csum_check;
 
 #ifdef CONFIG_PROC_FS
 	struct prot_inuse __percpu *prot_inuse;

net/core/dev.c +6 -0

index e76762e29360..d14950b1af3a 100644
@@ -2389,6 +2389,12 @@ EXPORT_SYMBOL(tcf_sw_enabled_key);
 
 DEFINE_STATIC_KEY_FALSE(netstamp_needed_key);
 EXPORT_SYMBOL(netstamp_needed_key);
+
+/* Enabled while any network namespace has net.core.virtio_net_hdr_csum_check
+ * set to 0.
+ */
+DEFINE_STATIC_KEY_FALSE(virtio_net_hdr_csum_check_disabled_key);
+EXPORT_SYMBOL(virtio_net_hdr_csum_check_disabled_key);
 #ifdef CONFIG_JUMP_LABEL
 static atomic_t netstamp_needed_deferred;
 static atomic_t netstamp_wanted;

net/core/net_namespace.c +1 -0

index da5f881fbd3b..2d340fafba85 100644
@@ -396,6 +396,7 @@ static __net_init void preinit_net_sysctl(struct net *net)
 	net->core.sysctl_optmem_max = 128 * 1024;
 	net->core.sysctl_txrehash = SOCK_TXREHASH_ENABLED;
 	net->core.sysctl_tstamp_allow_data = 1;
+	net->core.sysctl_virtio_net_hdr_csum_check = 1;
 	net->core.sysctl_txq_reselection = msecs_to_jiffies(1000);
 }
 

net/core/sysctl_net_core.c +1 -0

index eb35da3556f4..c65ff86cc5f9 100644
@@ -18,6 +18,7 @@
 #include <linux/slab.h>
 #include <linux/sched/isolation.h>
 #include <linux/hex.h>
+#include <linux/jump_label.h>
 
 #include <net/ip.h>
 #include <net/sock.h>

@@ -678,6 +679,54 @@ static struct ctl_table net_core_table[] = {
 	},
 };

net/core/sysctl_net_core.c +8 -0

+static DEFINE_MUTEX(virtio_net_hdr_csum_check_mutex);
+
+static int virtio_net_hdr_csum_check_sysctl(const struct ctl_table *table,
+					    int write, void *buffer,
+					    size_t *lenp, loff_t *ppos)
+{
+	struct net *net = (struct net *)table->data;
+	/* checkpatch warns but this can't be const: tmp.data is set

below */

net/core/sysctl_net_core.c +9 -0

+	u8 old;
+	int ret;
+
+	/* Root in a user namespace must not be able to weaken the
+	 * validation for the network namespace it controls.
+	 */
+	if (write && !capable(CAP_NET_ADMIN))
+		return -EPERM;
+

Quoted 167 lines · Michael S. Tsirkin

The ingress validation as implemented by the previous patch is stricter than what it replaced: GSO
packets whose checksum metadata matches no transport header are dropped, and non-GSO ones get their checksum completed in place. Such packets used to pass with the metadata unvalidated, and with no overhead.

I judge that this should not affect any deployments, but there is still some risk that it does. In case a deployment depends on them anyway, provide a per-netns switch to get the old behavior back:

echo 0 > /proc/sys/net/core/virtio_net_hdr_csum_check

The default is validating.

Assisted-by LLM

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 Documentation/admin-guide/sysctl/net.rst | 19 ++++++++
 include/linux/virtio_net.h               | 23 +++++++++
 include/net/netns/core.h                 |  1 +
 net/core/dev.c                           |  6 +++
 net/core/net_namespace.c                 |  1 +
 net/core/sysctl_net_core.c               | 60 ++++++++++++++++++++++++
 6 files changed, 110 insertions(+)

Documentation/admin-guide/sysctl/net.rst +19 -0

index e586e17fc7a5..b1a1c4714b22 100644
@@ -246,6 +246,25 @@ processes are dropped unless socket option SOF_TIMESTAMPING_OPT_TSONLY is set.
 Default: 1 (on)
 
 
+virtio_net_hdr_csum_check
+-------------------------
+
+When processing packets with a virtio net header (through tun, tap, a
+packet socket or a virtio network device) that request checksum offload
+(VIRTIO_NET_HDR_F_NEEDS_CSUM), validate the packet: complete the checksum
+in software, or drop the packet where the header does not match it.
+
+When disabled, the header is trusted not to corrupt the packet, and the
+checksum is offloaded to hardware (or not verified if it never reaches
+hardware). This is only safe if no protocol is processing the packets.
+
+The setting is per network namespace, but only writable by the host
+root: root inside a network namespace created by an unprivileged user
+cannot disable the validation.
+
+Default: 1 (on)
+
+
 wmem_default
 ------------
 

include/linux/virtio_net.h +23 -0

index 97b554ef7123..25aa95c11e31 100644
@@ -5,6 +5,7 @@
 #include <linux/if_vlan.h>
 #include <linux/ip.h>
 #include <linux/ipv6.h>
+#include <linux/jump_label.h>
 #include <linux/netdevice.h>
 #include <linux/udp.h>
 #include <net/gso.h>

@@ -78,6 +79,19 @@ static inline struct iphdr *virtio_net_ip4_parse(struct sk_buff *skb,
 	*thoff = nhoff + iph->ihl * sizeof(u32);
 	return iph;
 }
+/* Defined in net/core/dev.c, enabled while any network namespace
+ * has net.core.virtio_net_hdr_csum_check set to 0.
+ */
+DECLARE_STATIC_KEY_FALSE(virtio_net_hdr_csum_check_disabled_key);
+
+/* Failsafe: a namespace can disable the check with
+ * net.core.virtio_net_hdr_csum_check.
+ */
+static inline bool virtio_net_hdr_csum_check_disabled(struct sk_buff *skb)
+{
+	return static_branch_unlikely(&virtio_net_hdr_csum_check_disabled_key) &&
+	       !READ_ONCE(dev_net(skb->dev)->core.sysctl_virtio_net_hdr_csum_check);
+}
 
 /* Where does the L3 header start, and what protocol is it?
  * Handles Ethernet and VLAN headers; other devices use the

@@ -179,6 +193,12 @@ virtio_net_csum_partial_ok_at_offset(struct sk_buff *skb, u8 hdr_gso_type,
 	unsigned int thoff;
 	int ip_proto;
 
+	/* Failsafe: a namespace opted out of the validation,
+	 * trust the checksum metadata as before.
+	 */
+	if (virtio_net_hdr_csum_check_disabled(skb))
+		return true;
+
 	if (!virtio_net_l4_start(skb, nhoff, proto, &thoff, &ip_proto))
 		return false;
 

@@ -516,6 +536,9 @@ virtio_net_tnl_offsets_ok(struct sk_buff *skb, bool outer_isv6,
 	unsigned int thoff;
 	int ip_proto;
 
+	if (virtio_net_hdr_csum_check_disabled(skb))
+		return true;
+
 	if (!virtio_net_l3_start(skb, &nhoff, &proto))
 		return false;
 	if (outer_isv6 != (proto == htons(ETH_P_IPV6)))

include/net/netns/core.h +1 -0

index 9ef3d70e5e9c..ae8110772c97 100644
@@ -18,6 +18,7 @@ struct netns_core {
 	u8	sysctl_txrehash;
 	u8	sysctl_tstamp_allow_data;
 	u8	sysctl_bypass_prot_mem;
+	u8	sysctl_virtio_net_hdr_csum_check;
 
 #ifdef CONFIG_PROC_FS
 	struct prot_inuse __percpu *prot_inuse;

net/core/dev.c +6 -0

index e76762e29360..d14950b1af3a 100644
@@ -2389,6 +2389,12 @@ EXPORT_SYMBOL(tcf_sw_enabled_key);
 
 DEFINE_STATIC_KEY_FALSE(netstamp_needed_key);
 EXPORT_SYMBOL(netstamp_needed_key);
+
+/* Enabled while any network namespace has net.core.virtio_net_hdr_csum_check
+ * set to 0.
+ */
+DEFINE_STATIC_KEY_FALSE(virtio_net_hdr_csum_check_disabled_key);
+EXPORT_SYMBOL(virtio_net_hdr_csum_check_disabled_key);
 #ifdef CONFIG_JUMP_LABEL
 static atomic_t netstamp_needed_deferred;
 static atomic_t netstamp_wanted;

net/core/net_namespace.c +1 -0

index da5f881fbd3b..2d340fafba85 100644
@@ -396,6 +396,7 @@ static __net_init void preinit_net_sysctl(struct net *net)
 	net->core.sysctl_optmem_max = 128 * 1024;
 	net->core.sysctl_txrehash = SOCK_TXREHASH_ENABLED;
 	net->core.sysctl_tstamp_allow_data = 1;
+	net->core.sysctl_virtio_net_hdr_csum_check = 1;
 	net->core.sysctl_txq_reselection = msecs_to_jiffies(1000);
 }
 

net/core/sysctl_net_core.c +1 -0

index eb35da3556f4..c65ff86cc5f9 100644
@@ -18,6 +18,7 @@
 #include <linux/slab.h>
 #include <linux/sched/isolation.h>
 #include <linux/hex.h>
+#include <linux/jump_label.h>
 
 #include <net/ip.h>
 #include <net/sock.h>

@@ -678,6 +679,54 @@ static struct ctl_table net_core_table[] = {
 	},
 };

net/core/sysctl_net_core.c static struct ctl_table net_core_table[] = { · line 690

Quoted 10 lines

net/core/sysctl_net_core.c +9 -0

 
+/* Defined in net/core/dev.c, declared for the gate in
+ * include/linux/virtio_net.h.
+ */
+DECLARE_STATIC_KEY_FALSE(virtio_net_hdr_csum_check_disabled_key);
+
+/*
+ * Protects changes to virtio_net_hdr_csum_check_disabled_key and each
+ * net->core.sysctl_virtio_net_hdr_csum_check. Readers don't use a lock.
+ */

nit: OCD doesn't like mixing old netdev comments with vanilla style... one VVV as well.

Quoted 9 lines

net/core/sysctl_net_core.c +8 -0

+static DEFINE_MUTEX(virtio_net_hdr_csum_check_mutex);
+
+static int virtio_net_hdr_csum_check_sysctl(const struct ctl_table *table,
+					    int write, void *buffer,
+					    size_t *lenp, loff_t *ppos)
+{
+	struct net *net = (struct net *)table->data;
+	/* checkpatch warns but this can't be const: tmp.data is set

below */

nit: Redundant comment...

net/core/sysctl_net_core.c static struct ctl_table net_core_table[] = { · line 699

Quoted 1 line

net/core/sysctl_net_core.c +1 -0

+	struct ctl_table tmp = *table;

...and here, can't we instead get rid of tmp, and do something like:

    | u8 *value = table->data;
    | u8 old, new;

Quoted 9 lines

net/core/sysctl_net_core.c +9 -0

+	u8 old;
+	int ret;
+
+	/* Root in a user namespace must not be able to weaken the
+	 * validation for the network namespace it controls.
+	 */
+	if (write && !capable(CAP_NET_ADMIN))
+		return -EPERM;
+

net/core/sysctl_net_core.c static struct ctl_table net_core_table[] = { · line 718

Quoted 10 lines

net/core/sysctl_net_core.c +10 -0

+	/*
+	 * Make sure virtio_net_hdr_csum_check_disabled_key counts zero values of
+	 * net->core.sysctl_virtio_net_hdr_csum_check.
+	 */
+	mutex_lock(&virtio_net_hdr_csum_check_mutex);
+
+	old = net->core.sysctl_virtio_net_hdr_csum_check;
+	tmp.data = &net->core.sysctl_virtio_net_hdr_csum_check;
+	ret = proc_dou8vec_minmax(&tmp, write, buffer, lenp, ppos);
+	if (write && !ret && old != net->core.sysctl_virtio_net_hdr_csum_check) {

and then:

    | old = READ_ONCE(*value);
    | ret = proc_dou8vec_minmax(table, write, buffer, lenp, ppos);
    | new = READ_ONCE(*value);
    | 
    | if (write && !ret && old != new) {

at least that's easier to read for me. (No blocker!)

Reviewed-by Björn Töpel <[email protected]>

The ingress validation as implemented by the previous patch is stricter than what it replaced: GSO
packets whose checksum metadata matches no transport header are dropped, and non-GSO ones get their checksum completed in place. Such packets used to pass with the metadata unvalidated, and with no overhead.

I judge that this should not affect any deployments, but there is still some risk that it does. In case a deployment depends on them anyway, provide a per-netns switch to get the old behavior back:

echo 0 > /proc/sys/net/core/virtio_net_hdr_csum_check

The default is validating.

Assisted-by LLM

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 Documentation/admin-guide/sysctl/net.rst | 19 ++++++++
 include/linux/virtio_net.h               | 23 +++++++++
 include/net/netns/core.h                 |  1 +
 net/core/dev.c                           |  6 +++
 net/core/net_namespace.c                 |  1 +
 net/core/sysctl_net_core.c               | 60 ++++++++++++++++++++++++
 6 files changed, 110 insertions(+)

Documentation/admin-guide/sysctl/net.rst +0 -0

index e586e17fc7a5..b1a1c4714b22 100644

Documentation/admin-guide/sysctl/net.rst +17 -0

+
+When processing packets with a virtio net header (through tun, tap, a
+packet socket or a virtio network device) that request checksum offload
+(VIRTIO_NET_HDR_F_NEEDS_CSUM), validate the packet: complete the checksum
+in software, or drop the packet where the header does not match it.
+
+When disabled, the header is trusted not to corrupt the packet, and the
+checksum is offloaded to hardware (or not verified if it never reaches
+hardware). This is only safe if no protocol is processing the packets.
+
+The setting is per network namespace, but only writable by the host
+root: root inside a network namespace created by an unprivileged user
+cannot disable the validation.
+
+Default: 1 (on)
+
+
 wmem_default
 ------------
 

include/linux/virtio_net.h +14 -0

index 97b554ef7123..25aa95c11e31 100644
@@ -5,6 +5,7 @@
 #include <linux/if_vlan.h>
 #include <linux/ip.h>
 #include <linux/ipv6.h>
+#include <linux/jump_label.h>
 #include <linux/netdevice.h>
 #include <linux/udp.h>
 #include <net/gso.h>

@@ -78,6 +79,19 @@ static inline struct iphdr *virtio_net_ip4_parse(struct sk_buff *skb,
 	*thoff = nhoff + iph->ihl * sizeof(u32);
 	return iph;
 }
+/* Defined in net/core/dev.c, enabled while any network namespace
+ * has net.core.virtio_net_hdr_csum_check set to 0.
+ */
+DECLARE_STATIC_KEY_FALSE(virtio_net_hdr_csum_check_disabled_key);
+
+/* Failsafe: a namespace can disable the check with
+ * net.core.virtio_net_hdr_csum_check.
+ */
+static inline bool virtio_net_hdr_csum_check_disabled(struct sk_buff *skb)
+{
+	return static_branch_unlikely(&virtio_net_hdr_csum_check_disabled_key) &&
+	       !READ_ONCE(dev_net(skb->dev)->core.sysctl_virtio_net_hdr_csum_check);
+}
 
 /* Where does the L3 header start, and what protocol is it?
  * Handles Ethernet and VLAN headers; other devices use the

include/linux/virtio_net.h +4 -0

+
 	if (!virtio_net_l4_start(skb, nhoff, proto, &thoff, &ip_proto))
 		return false;
 

@@ -516,6 +536,9 @@ virtio_net_tnl_offsets_ok(struct sk_buff *skb, bool outer_isv6,
 	unsigned int thoff;
 	int ip_proto;
 
+	if (virtio_net_hdr_csum_check_disabled(skb))
+		return true;
+
 	if (!virtio_net_l3_start(skb, &nhoff, &proto))
 		return false;
 	if (outer_isv6 != (proto == htons(ETH_P_IPV6)))

include/net/netns/core.h +1 -0

index 9ef3d70e5e9c..ae8110772c97 100644
@@ -18,6 +18,7 @@ struct netns_core {
 	u8	sysctl_txrehash;
 	u8	sysctl_tstamp_allow_data;
 	u8	sysctl_bypass_prot_mem;
+	u8	sysctl_virtio_net_hdr_csum_check;
 
 #ifdef CONFIG_PROC_FS
 	struct prot_inuse __percpu *prot_inuse;

net/core/dev.c +6 -0

index e76762e29360..d14950b1af3a 100644
@@ -2389,6 +2389,12 @@ EXPORT_SYMBOL(tcf_sw_enabled_key);
 
 DEFINE_STATIC_KEY_FALSE(netstamp_needed_key);
 EXPORT_SYMBOL(netstamp_needed_key);
+
+/* Enabled while any network namespace has net.core.virtio_net_hdr_csum_check
+ * set to 0.
+ */
+DEFINE_STATIC_KEY_FALSE(virtio_net_hdr_csum_check_disabled_key);
+EXPORT_SYMBOL(virtio_net_hdr_csum_check_disabled_key);
 #ifdef CONFIG_JUMP_LABEL
 static atomic_t netstamp_needed_deferred;
 static atomic_t netstamp_wanted;

net/core/net_namespace.c +1 -0

index da5f881fbd3b..2d340fafba85 100644
@@ -396,6 +396,7 @@ static __net_init void preinit_net_sysctl(struct net *net)
 	net->core.sysctl_optmem_max = 128 * 1024;
 	net->core.sysctl_txrehash = SOCK_TXREHASH_ENABLED;
 	net->core.sysctl_tstamp_allow_data = 1;
+	net->core.sysctl_virtio_net_hdr_csum_check = 1;
 	net->core.sysctl_txq_reselection = msecs_to_jiffies(1000);
 }
 

net/core/sysctl_net_core.c +1 -0

index eb35da3556f4..c65ff86cc5f9 100644
@@ -18,6 +18,7 @@
 #include <linux/slab.h>
 #include <linux/sched/isolation.h>
 #include <linux/hex.h>
+#include <linux/jump_label.h>
 
 #include <net/ip.h>
 #include <net/sock.h>

@@ -678,6 +679,54 @@ static struct ctl_table net_core_table[] = {
 	},

net/core/sysctl_net_core.c +40 -0

+ */
+static DEFINE_MUTEX(virtio_net_hdr_csum_check_mutex);
+
+static int virtio_net_hdr_csum_check_sysctl(const struct ctl_table *table,
+					    int write, void *buffer,
+					    size_t *lenp, loff_t *ppos)
+{
+	struct net *net = (struct net *)table->data;
+	/* checkpatch warns but this can't be const: tmp.data is set below */
+	struct ctl_table tmp = *table;
+	u8 old;
+	int ret;
+
+	/* Root in a user namespace must not be able to weaken the
+	 * validation for the network namespace it controls.
+	 */
+	if (write && !capable(CAP_NET_ADMIN))
+		return -EPERM;
+
+	/*
+	 * Make sure virtio_net_hdr_csum_check_disabled_key counts zero values of
+	 * net->core.sysctl_virtio_net_hdr_csum_check.
+	 */
+	mutex_lock(&virtio_net_hdr_csum_check_mutex);
+
+	old = net->core.sysctl_virtio_net_hdr_csum_check;
+	tmp.data = &net->core.sysctl_virtio_net_hdr_csum_check;
+	ret = proc_dou8vec_minmax(&tmp, write, buffer, lenp, ppos);
+	if (write && !ret && old != net->core.sysctl_virtio_net_hdr_csum_check) {
+		if (net->core.sysctl_virtio_net_hdr_csum_check)
+			static_branch_dec(&virtio_net_hdr_csum_check_disabled_key);
+		else
+			static_branch_inc(&virtio_net_hdr_csum_check_disabled_key);
+	}
+
+	mutex_unlock(&virtio_net_hdr_csum_check_mutex);
+
+	return ret;
+}
+
 static const struct ctl_table netns_core_table[] = {
 #if IS_ENABLED(CONFIG_RPS)
 	{

net/core/sysctl_net_core.c +8 -0

+		.maxlen		= sizeof(u8),
+		.mode		= 0644,
+		.proc_handler	= virtio_net_hdr_csum_check_sysctl,
+		.extra1		= SYSCTL_ZERO,
+		.extra2		= SYSCTL_ONE
+	},
 	/* sysctl_core_net_init() will set the values after this
 	 * to readonly in network namespaces
 	 */

@@ -841,6 +899,8 @@ static __net_exit void sysctl_core_net_exit(struct net *net)
 	tbl = net->core.sysctl_hdr->ctl_table_arg;
 	unregister_net_sysctl_table(net->core.sysctl_hdr);
 	BUG_ON(tbl == netns_core_table);
+	if (!net->core.sysctl_virtio_net_hdr_csum_check)
+		static_branch_dec(&virtio_net_hdr_csum_check_disabled_key);
 #if IS_ENABLED(CONFIG_RPS)
 	kfree(net->core.rps_default_mask);
 #endif

Signature · 1 line

MST

Quoted 34 lines · Michael S. Tsirkin

The ingress validation as implemented by the previous patch is stricter than what it replaced: GSO
packets whose checksum metadata matches no transport header are dropped, and non-GSO ones get their checksum completed in place. Such packets used to pass with the metadata unvalidated, and with no overhead.

I judge that this should not affect any deployments, but there is still some risk that it does. In case a deployment depends on them anyway, provide a per-netns switch to get the old behavior back:

echo 0 > /proc/sys/net/core/virtio_net_hdr_csum_check

The default is validating.

Assisted-by LLM

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 Documentation/admin-guide/sysctl/net.rst | 19 ++++++++
 include/linux/virtio_net.h               | 23 +++++++++
 include/net/netns/core.h                 |  1 +
 net/core/dev.c                           |  6 +++
 net/core/net_namespace.c                 |  1 +
 net/core/sysctl_net_core.c               | 60 ++++++++++++++++++++++++
 6 files changed, 110 insertions(+)

Documentation/admin-guide/sysctl/net.rst +0 -0

index e586e17fc7a5..b1a1c4714b22 100644
@@ -246,6 +246,25 @@ processes are dropped unless socket option SOF_TIMESTAMPING_OPT_TSONLY is set.
 Default: 1 (on)
 
 

Documentation/admin-guide/sysctl/net.rst processes are dropped unless socket option SOF_TIMESTAMPING_OPT_TSONLY is set. · line 250

Quoted 2 lines

Documentation/admin-guide/sysctl/net.rst +2 -0

+virtio_net_hdr_csum_check
+-------------------------

virtio_net_hdr_validate? In case we do have to add more validation later.