On Thu, Sep 21, 2023 at 7:09 AM Eric Dumazet <[email protected]> wrote:
>
> On Thu, Sep 21, 2023 at 12:41 PM David Howells <[email protected]> wrote:
> >
> >
> > Including the transhdrlen in length is a problem when the packet is
> > partially filled (e.g. something like send(MSG_MORE) happened previously)
> > when appending to an IPv4 or IPv6 packet as we don't want to repeat the
> > transport header or account for it twice. This can happen under some
> > circumstances, such as splicing into an L2TP socket.
> >
> > The symptom observed is a warning in __ip6_append_data():
> >
> > WARNING: CPU: 1 PID: 5042 at net/ipv6/ip6_output.c:1800 __ip6_append_data.isra.0+0x1be8/0x47f0 net/ipv6/ip6_output.c:1800
> >
> > that occurs when MSG_SPLICE_PAGES is used to append more data to an already
> > partially occupied skbuff. The warning occurs when 'copy' is larger than
> > the amount of data in the message iterator. This is because the requested
> > length includes the transport header length when it shouldn't. This can be
> > triggered by, for example:
> >
> > sfd = socket(AF_INET6, SOCK_DGRAM, IPPROTO_L2TP);
> > bind(sfd, ...); // ::1
> > connect(sfd, ...); // ::1 port 7
> > send(sfd, buffer, 4100, MSG_MORE);
> > sendfile(sfd, dfd, NULL, 1024);
> >
> > Fix this by only adding transhdrlen into the length if the write queue is
> > empty in l2tp_ip6_sendmsg(), analogously to how UDP does things.
> >
> > l2tp_ip_sendmsg() looks like it won't suffer from this problem as it builds
> > the UDP packet itself.
> >
> > Fixes: a32e0eec7042 ("l2tp: introduce L2TPv3 IP encapsulation support for IPv6")
> > Reported-by: [email protected]
> > Link: https://lore.kernel.org/r/[email protected]/
> > Suggested-by: Willem de Bruijn <[email protected]>
> > Signed-off-by: David Howells <[email protected]>
> > cc: Eric Dumazet <[email protected]>
> > cc: Willem de Bruijn <[email protected]>
> > cc: "David S. Miller" <[email protected]>
> > cc: David Ahern <[email protected]>
> > cc: Paolo Abeni <[email protected]>
> > cc: Jakub Kicinski <[email protected]>
> > cc: [email protected]
> > cc: [email protected]
> > cc: [email protected]
> > ---
>
> Looks safer indeed, thanks to you and Willem !
>
> Reviewed-by: Eric Dumazet <[email protected]>
Reviewed-by: Willem de Bruijn <[email protected]>