Re: sendmsg bug

Re: sendmsg bug

[Linus Torvalds]

[ Appending the full original email - converted to plain-text - below.
Note that the "comments i marked red" are no longer red, since the
html has been stripped out ]

Dear 1031265646,
 please don't email me in private about bugs, much better to send them
to the right person and mailing list (see scripts/get_maintainer.pl in
the kernel sources).

The bug seems real, although mostly harmless. If ip_make_skb() returns
NULL, it's true that udp_sendmsg() will return a misleading success
value since 'err' will be 0 and it will return the length of the
packet that wasn't actually ever created or sent.

UDP being a lossy protocol, probably nobody has cared, since
"successful send" doesn't mean "successful receive" anyway.

I'm not sure what the right error should be for this case, and whether
it should be fixed inside ip_make_skb() ("always return a proper
ERR_PTR") or what.. Or whether it should just be left alone as a
"packet dropped early" thing.

I'll leave that to the network people added to the participants.

            Linus

On Thu, Mar 3, 2022 at 10:23 PM 1031265646 <1031265646@qq.com> wrote:
>
> hi，
>
> in file udp.c, a function named udp_sendmsg has a code like this:
>
> /* Lockless fast path for the non-corking case. */
> if (!corkreq) {
> skb = ip_make_skb(sk, fl4, getfrag, msg, ulen,
>  sizeof(struct udphdr), &ipc, &rt,
>  msg->msg_flags);
> err = PTR_ERR(skb);
>
>         //here IS_ERR_OR_NULL is expected, instead of !IS_ERR_OR_NULL.
> if (!IS_ERR_OR_NULL(skb))
> err = udp_send_skb(skb, fl4);
> goto out;
> }
>
> but function ip_make_skb may return a null, then err will be set to 0;and out like this:
>
> out:
> ip_rt_put(rt);
> if (free)
> kfree(ipc.opt);
> if (!err)
> return len;  // return a positive value
>
> the ip_make_skb failed means the send operation failed. but a positive value is returnd here. finnally, users regard the operation was success, but actually it failed in kernel.
>
>
> the comments i marked red is the right way i think. which i think is a bug.
>
>
> 1031265646
> 1031265646@qq.com
> 签名由网易邮箱大师定制

Re: sendmsg bug

[David Ahern]

On 3/4/22 2:13 PM, Linus Torvalds wrote:
> The bug seems real, although mostly harmless. If ip_make_skb() returns
> NULL, it's true that udp_sendmsg() will return a misleading success
> value since 'err' will be 0 and it will return the length of the
> packet that wasn't actually ever created or sent.
> 
> UDP being a lossy protocol, probably nobody has cared, since
> "successful send" doesn't mean "successful receive" anyway.
> 
> I'm not sure what the right error should be for this case, and whether
> it should be fixed inside ip_make_skb() ("always return a proper
> ERR_PTR") or what.. Or whether it should just be left alone as a
> "packet dropped early" thing.

I think the right answer is to have ip_make_skb (and by extension
__ip_make_skb) always return an skb or an ERR_PTR. That means fixing up
the callers and making sure err is set properly for the various paths.

Way too late in 5.17 for such a patch, but I can take look for 5.18. The
bug has been around since 2011 (903ab86d195c).