[PATCH net] net/packet: preserve TX_RING progress on a later frame error

[PATCH net] net/packet: preserve TX_RING progress on a later frame error

[Mark Amirkan via B4 Relay]

From: Mark Amirkan <markdamirkan@gmail.com>

tpacket_snd() can transmit one or more frames before a later frame fails
validation.  The failing frame is marked TP_STATUS_WRONG_FORMAT, but its
error replaces len_sum, so send() reports failure despite the earlier
transmission.

Return the completed byte count when it is nonzero, as the allocation
failure path already does.  Keep TP_STATUS_WRONG_FORMAT on the bad frame
so userspace can identify it.

In a two-frame TPACKET_V2 test, a valid 60-byte frame followed by an
oversized frame sends the first frame but returns -EMSGSIZE.  With this
change, send() returns 60 and the second frame remains marked
TP_STATUS_WRONG_FORMAT.

Fixes: 69e3c75f4d54 ("net: TX_RING and packet mmap")
Cc: stable@vger.kernel.org
Assisted-by: Symbolic
Signed-off-by: Mark Amirkan <markdamirkan@gmail.com>
---
 net/packet/af_packet.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 76bde7906d..b7e1848b61 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -2893,7 +2893,7 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
 				continue;
 			} else {
 				status = TP_STATUS_WRONG_FORMAT;
-				err = tp_len;
+				err = len_sum ? : tp_len;
 				goto out_status;
 			}
 		}

---
base-commit: e6b6078ea1731b05b3b552497b3bce4bf8b014ae
change-id: 20260913-b4-send-packet-tx-progress-2b0c8b6b4596

Best regards,
--  
Mark Amirkan <markdamirkan@gmail.com>

Re: [PATCH net] net/packet: preserve TX_RING progress on a later frame error

[Willem de Bruijn]

Mark Amirkan via B4 Relay wrote:
> From: Mark Amirkan <markdamirkan@gmail.com>
> 
> tpacket_snd() can transmit one or more frames before a later frame fails
> validation.  The failing frame is marked TP_STATUS_WRONG_FORMAT, but its
> error replaces len_sum, so send() reports failure despite the earlier
> transmission.
> 
> Return the completed byte count when it is nonzero, as the allocation
> failure path already does.  Keep TP_STATUS_WRONG_FORMAT on the bad frame
> so userspace can identify it.
> 
> In a two-frame TPACKET_V2 test, a valid 60-byte frame followed by an
> oversized frame sends the first frame but returns -EMSGSIZE.  With this
> change, send() returns 60 and the second frame remains marked
> TP_STATUS_WRONG_FORMAT.
> 
> Fixes: 69e3c75f4d54 ("net: TX_RING and packet mmap")
> Cc: stable@vger.kernel.org
> Assisted-by: Symbolic
> Signed-off-by: Mark Amirkan <markdamirkan@gmail.com>
> ---
>  net/packet/af_packet.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index 76bde7906d..b7e1848b61 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -2893,7 +2893,7 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
>  				continue;
>  			} else {
>  				status = TP_STATUS_WRONG_FORMAT;
> -				err = tp_len;
> +				err = len_sum ? : tp_len;
>  				goto out_status;
>  			}

This makes sense in principle, but changes longtime established and
expected behavior.

In particular, applications may not know to recover from a
TP_STATUS_WRONG_FORMAT unless an error is returned.

If this sendmsg returns tp_len here, i.e., (partial) success,
subsequent calls will return 0 / -ETIMEDOUT, as if no space is
available.

Re: [PATCH net] net/packet: preserve TX_RING progress on a later frame error

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 0 · Low: 0

- [High] Incomplete fix: the bug class named in the commit subject
  ("preserve TX_RING progress on a later frame error") stays reachable…

--

net/packet: preserve TX_RING progress on a later frame error

This changes the frame validation error path in tpacket_snd() so that the
already-transmitted byte count is returned instead of the per-frame error
when at least one frame has already been sent.  The failing frame is still
marked TP_STATUS_WRONG_FORMAT.

> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index 76bde7906d494..b7e1848b61865 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -2893,7 +2893,7 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
>  				continue;
>  			} else {
>  				status = TP_STATUS_WRONG_FORMAT;
> -				err = tp_len;
> +				err = len_sum ? : tp_len;
>  				goto out_status;
>  			}
>  		}

[Severity: High]
The commit subject says "preserve TX_RING progress on a later frame error",
but only the validation error exit is converted.  Are the other exits of
tpacket_snd() that can also be reached with len_sum > 0 meant to keep
returning the errno?

len_sum is only accumulated at the bottom of the loop, after the frame has
been handed to packet_xmit():

net/packet/af_packet.c:tpacket_snd() {
	...
		packet_increment_head(&po->tx_ring);
		len_sum += tp_len;
	} while (1);
	...
}

so on any iteration after the first, the transmit failure exit is reachable
with len_sum > 0 and still overwrites it:

net/packet/af_packet.c:tpacket_snd() {
	...
		err = packet_xmit(po, skb);
		if (unlikely(err != 0)) {
			if (err > 0)
				err = net_xmit_errno(err);
			if (err && __packet_get_status(po, ph) ==
				   TP_STATUS_AVAILABLE) {
				/* skb was destructed already */
				skb = NULL;
				goto out_status;
			}
	...
}

Here err carries the negative xmit errno (for example -ENOBUFS derived from
NET_XMIT_DROP, or -ENETDOWN) and len_sum is discarded, so send() reports
failure even though earlier frames were already handed to the device.  Isn't
that the same behaviour the changelog describes as wrong for the validation
path?

The wait path has the same shape:

net/packet/af_packet.c:tpacket_snd() {
	...
			if (need_wait && packet_read_pending(&po->tx_ring)) {
				timeo = wait_for_completion_interruptible_timeout(&po->skb_completion, timeo);
				if (timeo <= 0) {
					err = !timeo ? -ETIMEDOUT : -ERESTARTSYS;
					goto out_put;
				}
	...
}

On a blocking socket with SO_SNDTIMEO, once frame N has been transmitted and
the head frame is no longer TP_STATUS_SEND_REQUEST while completions are
still pending, this returns -ETIMEDOUT or -ERESTARTSYS with len_sum > 0.

With the patch applied, the four exits that can see len_sum > 0 disagree:
the sock_alloc_send_skb() failure path returns len_sum, the validation path
now returns len_sum, and the packet_xmit() failure and wait-timeout paths
return a negative errno.  Can userspace still derive a single interpretation
of the send()/sendto() return value for a PF_PACKET TX_RING socket?

Would it make sense to either apply the same treatment to the xmit and
timeout exits, or narrow the changelog to the validation error and state why
the remaining exits intentionally keep reporting the errno?

The changelog also says:

> Return the completed byte count when it is nonzero, as the allocation
> failure path already does.

The allocation failure path uses "if (likely(len_sum > 0))" while the new
code uses "len_sum ? :", which also takes the len_sum branch for a negative
len_sum.  Is the difference intentional?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260913-b4-send-packet-tx-progress-v1-1-01b99569cda6%40gmail.com