[PATCH net] tcp: devmem: properly export MSG_CTRUNC to userspace

[PATCH net] tcp: devmem: properly export MSG_CTRUNC to userspace

[Stanislav Fomichev]

Currently, we report -ETOOSMALL (err) only on the first iteration
(!sent). When we get put_cmsg error after a bunch of successful
put_cmsg calls, we don't signal the error at all. This might be
confusing on the userspace side which will see truncated CMSGs
but no MSG_CTRUNC signal.

Consider the following case:
- sizeof(struct cmsghdr) = 16
- sizeof(struct dmabuf_cmsg) = 24
- total cmsg size (CMSG_LEN) = 40 (16+24)

When calling recvmsg with msg_controllen=60, the userspace
will receive two(!) dmabuf_cmsg(s), the first one will
be a valid one and the second one will be silently truncated. There is no
easy way to discover the truncation besides doing something like
"cm->cmsg_len != CMSG_LEN(sizeof(dmabuf_cmsg))". Do not mask MSG_CTRUNC
to keep conventional cmsg-truncated signals in place.

If there is a concern with breaking UAPI, I can document this case in
devmem.rst instead.

Fixes: 8f0b3cc9a4c1 ("tcp: RX path for devmem TCP")
Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
---
 net/ipv4/tcp.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 0d704bda6c41..bdc9ac648d83 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -2441,7 +2441,6 @@ static int tcp_recvmsg_dmabuf(struct sock *sk, const struct sk_buff *skb,
 			err = put_cmsg(msg, SOL_SOCKET, SO_DEVMEM_LINEAR,
 				       sizeof(dmabuf_cmsg), &dmabuf_cmsg);
 			if (err || msg->msg_flags & MSG_CTRUNC) {
-				msg->msg_flags &= ~MSG_CTRUNC;
 				if (!err)
 					err = -ETOOSMALL;
 				goto out;
@@ -2504,7 +2503,6 @@ static int tcp_recvmsg_dmabuf(struct sock *sk, const struct sk_buff *skb,
 					       sizeof(dmabuf_cmsg),
 					       &dmabuf_cmsg);
 				if (err || msg->msg_flags & MSG_CTRUNC) {
-					msg->msg_flags &= ~MSG_CTRUNC;
 					if (!err)
 						err = -ETOOSMALL;
 					goto out;
-- 
2.48.1

Re: [PATCH net] tcp: devmem: properly export MSG_CTRUNC to userspace

[Mina Almasry]

On Tue, Feb 18, 2025 at 11:40 AM Stanislav Fomichev <sdf@fomichev.me> wrote:
>
> Currently, we report -ETOOSMALL (err) only on the first iteration
> (!sent). When we get put_cmsg error after a bunch of successful
> put_cmsg calls, we don't signal the error at all. This might be
> confusing on the userspace side which will see truncated CMSGs
> but no MSG_CTRUNC signal.
>
> Consider the following case:
> - sizeof(struct cmsghdr) = 16
> - sizeof(struct dmabuf_cmsg) = 24
> - total cmsg size (CMSG_LEN) = 40 (16+24)
>
> When calling recvmsg with msg_controllen=60, the userspace
> will receive two(!) dmabuf_cmsg(s), the first one will

The intended API in this scenario is that the user will receive *one*
dmabuf_cmgs. The kernel will consider that data in that frag to be
delivered to userspace, and subsequent recvmsg() calls will not
re-deliver that data. The next recvmsg() call will deliver the data
that we failed to put_cmsg() in the current call.

If you receive two dmabuf_cmsgs in this scenario, that is indeed a
bug. Exposing CMSG_CTRUNC could be a good fix. It may indicate to the
user "ignore the last cmsg we put, because it got truncated, and
you'll receive the full cmsg on the next recvmsg call". We do need to
update the docs for this I think.

However, I think a much much better fix is to modify put_cmsg() so
that we only get one dmabuf_cmsgs in this scenario, if possible. We
could add a strict flag to put_cmsg(). If (strict == true &&
msg->controlllen < cmlen), we return an error instead of putting a
truncated cmsg, so that the user only sees one dmabuf_cmsg in this
scenario.

Is this doable?

--
Thanks,
Mina

Re: [PATCH net] tcp: devmem: properly export MSG_CTRUNC to userspace

[Stanislav Fomichev]

On 02/18, Mina Almasry wrote:
> On Tue, Feb 18, 2025 at 11:40 AM Stanislav Fomichev <sdf@fomichev.me> wrote:
> >
> > Currently, we report -ETOOSMALL (err) only on the first iteration
> > (!sent). When we get put_cmsg error after a bunch of successful
> > put_cmsg calls, we don't signal the error at all. This might be
> > confusing on the userspace side which will see truncated CMSGs
> > but no MSG_CTRUNC signal.
> >
> > Consider the following case:
> > - sizeof(struct cmsghdr) = 16
> > - sizeof(struct dmabuf_cmsg) = 24
> > - total cmsg size (CMSG_LEN) = 40 (16+24)
> >
> > When calling recvmsg with msg_controllen=60, the userspace
> > will receive two(!) dmabuf_cmsg(s), the first one will
> 
> The intended API in this scenario is that the user will receive *one*
> dmabuf_cmgs. The kernel will consider that data in that frag to be
> delivered to userspace, and subsequent recvmsg() calls will not
> re-deliver that data. The next recvmsg() call will deliver the data
> that we failed to put_cmsg() in the current call.
> 
> If you receive two dmabuf_cmsgs in this scenario, that is indeed a
> bug. Exposing CMSG_CTRUNC could be a good fix. It may indicate to the
> user "ignore the last cmsg we put, because it got truncated, and
> you'll receive the full cmsg on the next recvmsg call". We do need to
> update the docs for this I think.
> 
> However, I think a much much better fix is to modify put_cmsg() so
> that we only get one dmabuf_cmsgs in this scenario, if possible. We
> could add a strict flag to put_cmsg(). If (strict == true &&
> msg->controlllen < cmlen), we return an error instead of putting a
> truncated cmsg, so that the user only sees one dmabuf_cmsg in this
> scenario.
> 
> Is this doable?

Instead of modifying put_cmsg(), I can have an extra check before
calling it to make sure the full entry fits. Something like:

--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -2498,6 +2498,11 @@ static int tcp_recvmsg_dmabuf(struct sock *sk, const struct sk_buff *skb,
 				offset += copy;
 				remaining_len -= copy;
 
+				if (msg.msg_controllen < CMSG_LEN(sizeof(dmabuf_cmsg))) {
+					err = -ETOOSMALL;
+					goto out;
+				}
+
 				err = put_cmsg(msg, SOL_SOCKET,
 					       SO_DEVMEM_DMABUF,
 					       sizeof(dmabuf_cmsg),

WDYT? I'll still probably remove '~MSG_CTRUNC' parts as well to avoid
confusion.

Re: [PATCH net] tcp: devmem: properly export MSG_CTRUNC to userspace

[Mina Almasry]

On Tue, Feb 18, 2025 at 1:17 PM Stanislav Fomichev <stfomichev@gmail.com> wrote:
>
> On 02/18, Mina Almasry wrote:
> > On Tue, Feb 18, 2025 at 11:40 AM Stanislav Fomichev <sdf@fomichev.me> wrote:
> > >
> > > Currently, we report -ETOOSMALL (err) only on the first iteration
> > > (!sent). When we get put_cmsg error after a bunch of successful
> > > put_cmsg calls, we don't signal the error at all. This might be
> > > confusing on the userspace side which will see truncated CMSGs
> > > but no MSG_CTRUNC signal.
> > >
> > > Consider the following case:
> > > - sizeof(struct cmsghdr) = 16
> > > - sizeof(struct dmabuf_cmsg) = 24
> > > - total cmsg size (CMSG_LEN) = 40 (16+24)
> > >
> > > When calling recvmsg with msg_controllen=60, the userspace
> > > will receive two(!) dmabuf_cmsg(s), the first one will
> >
> > The intended API in this scenario is that the user will receive *one*
> > dmabuf_cmgs. The kernel will consider that data in that frag to be
> > delivered to userspace, and subsequent recvmsg() calls will not
> > re-deliver that data. The next recvmsg() call will deliver the data
> > that we failed to put_cmsg() in the current call.
> >
> > If you receive two dmabuf_cmsgs in this scenario, that is indeed a
> > bug. Exposing CMSG_CTRUNC could be a good fix. It may indicate to the
> > user "ignore the last cmsg we put, because it got truncated, and
> > you'll receive the full cmsg on the next recvmsg call". We do need to
> > update the docs for this I think.
> >
> > However, I think a much much better fix is to modify put_cmsg() so
> > that we only get one dmabuf_cmsgs in this scenario, if possible. We
> > could add a strict flag to put_cmsg(). If (strict == true &&
> > msg->controlllen < cmlen), we return an error instead of putting a
> > truncated cmsg, so that the user only sees one dmabuf_cmsg in this
> > scenario.
> >
> > Is this doable?
>
> Instead of modifying put_cmsg(), I can have an extra check before
> calling it to make sure the full entry fits. Something like:
>

Yes, that sounds perfect. I would add a new helper, maybe
put_dmabuf_cmsg, that checks that we have enough space before calling
the generic put_cmsg().

> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -2498,6 +2498,11 @@ static int tcp_recvmsg_dmabuf(struct sock *sk, const struct sk_buff *skb,
>                                 offset += copy;
>                                 remaining_len -= copy;
>
> +                               if (msg.msg_controllen < CMSG_LEN(sizeof(dmabuf_cmsg))) {
> +                                       err = -ETOOSMALL;
> +                                       goto out;
> +                               }
> +
>                                 err = put_cmsg(msg, SOL_SOCKET,
>                                                SO_DEVMEM_DMABUF,
>                                                sizeof(dmabuf_cmsg),
>
> WDYT? I'll still probably remove '~MSG_CTRUNC' parts as well to avoid
> confusion.

Yes, since we check there is enough space before calling put_cmsg(),
it should now become impossible for put_cmsg() to set MSG_CTRUNC
anyway, so the check in tcp_recvmsg_dmabuf() becomes an unnecessary
defensive check that should be removed.

Thanks for catching this!

-- 
Thanks,
Mina

Re: [PATCH net] tcp: devmem: properly export MSG_CTRUNC to userspace

[Stanislav Fomichev]

On 02/18, Mina Almasry wrote:
> On Tue, Feb 18, 2025 at 1:17 PM Stanislav Fomichev <stfomichev@gmail.com> wrote:
> >
> > On 02/18, Mina Almasry wrote:
> > > On Tue, Feb 18, 2025 at 11:40 AM Stanislav Fomichev <sdf@fomichev.me> wrote:
> > > >
> > > > Currently, we report -ETOOSMALL (err) only on the first iteration
> > > > (!sent). When we get put_cmsg error after a bunch of successful
> > > > put_cmsg calls, we don't signal the error at all. This might be
> > > > confusing on the userspace side which will see truncated CMSGs
> > > > but no MSG_CTRUNC signal.
> > > >
> > > > Consider the following case:
> > > > - sizeof(struct cmsghdr) = 16
> > > > - sizeof(struct dmabuf_cmsg) = 24
> > > > - total cmsg size (CMSG_LEN) = 40 (16+24)
> > > >
> > > > When calling recvmsg with msg_controllen=60, the userspace
> > > > will receive two(!) dmabuf_cmsg(s), the first one will
> > >
> > > The intended API in this scenario is that the user will receive *one*
> > > dmabuf_cmgs. The kernel will consider that data in that frag to be
> > > delivered to userspace, and subsequent recvmsg() calls will not
> > > re-deliver that data. The next recvmsg() call will deliver the data
> > > that we failed to put_cmsg() in the current call.
> > >
> > > If you receive two dmabuf_cmsgs in this scenario, that is indeed a
> > > bug. Exposing CMSG_CTRUNC could be a good fix. It may indicate to the
> > > user "ignore the last cmsg we put, because it got truncated, and
> > > you'll receive the full cmsg on the next recvmsg call". We do need to
> > > update the docs for this I think.
> > >
> > > However, I think a much much better fix is to modify put_cmsg() so
> > > that we only get one dmabuf_cmsgs in this scenario, if possible. We
> > > could add a strict flag to put_cmsg(). If (strict == true &&
> > > msg->controlllen < cmlen), we return an error instead of putting a
> > > truncated cmsg, so that the user only sees one dmabuf_cmsg in this
> > > scenario.
> > >
> > > Is this doable?
> >
> > Instead of modifying put_cmsg(), I can have an extra check before
> > calling it to make sure the full entry fits. Something like:
> >
> 
> Yes, that sounds perfect. I would add a new helper, maybe
> put_dmabuf_cmsg, that checks that we have enough space before calling
> the generic put_cmsg().
> 
> > --- a/net/ipv4/tcp.c
> > +++ b/net/ipv4/tcp.c
> > @@ -2498,6 +2498,11 @@ static int tcp_recvmsg_dmabuf(struct sock *sk, const struct sk_buff *skb,
> >                                 offset += copy;
> >                                 remaining_len -= copy;
> >
> > +                               if (msg.msg_controllen < CMSG_LEN(sizeof(dmabuf_cmsg))) {
> > +                                       err = -ETOOSMALL;
> > +                                       goto out;
> > +                               }
> > +
> >                                 err = put_cmsg(msg, SOL_SOCKET,
> >                                                SO_DEVMEM_DMABUF,
> >                                                sizeof(dmabuf_cmsg),
> >
> > WDYT? I'll still probably remove '~MSG_CTRUNC' parts as well to avoid
> > confusion.
> 
> Yes, since we check there is enough space before calling put_cmsg(),
> it should now become impossible for put_cmsg() to set MSG_CTRUNC
> anyway, so the check in tcp_recvmsg_dmabuf() becomes an unnecessary
> defensive check that should be removed.
> 
> Thanks for catching this!

Perfect, thanks for a quick review!

---
pw-bot: cr