On Tue, May 15, 2018 at 4:04 PM, Willem de Bruijn
<willemdebruijn.ker...@gmail.com> wrote:
> On Tue, May 15, 2018 at 10:14 AM, Willem de Bruijn
> <willemdebruijn.ker...@gmail.com> wrote:
>> On Mon, May 14, 2018 at 7:45 PM, Eric Dumazet <eric.duma...@gmail.com> wrote:
>>>
>>>
>>> On 05/14/2018 04:30 PM, Willem de Bruijn wrote:
>>>
>>>> I don't quite follow. The reported crash happens in the protocol layer,
>>>> because of this check. With pagedlen we have not allocated
>>>> sufficient space for the skb_put.
>>>>
>>>> if (!(rt->dst.dev->features&NETIF_F_SG)) {
>>>> unsigned int off;
>>>>
>>>> off = skb->len;
>>>> if (getfrag(from, skb_put(skb, copy),
>>>> offset, copy, off, skb) < 0) {
>>>> __skb_trim(skb, off);
>>>> err = -EFAULT;
>>>> goto error;
>>>> }
>>>> } else {
>>>> int i = skb_shinfo(skb)->nr_frags;
>>>>
>>>> Are you referring to a separate potential issue in the gso layer?
>>>> If a bonding device advertises SG, but a slave does not, then
>>>> skb_segment on the slave should build linear segs? I have not
>>>> tested that.
>>>
>>> Given that the device attribute could change under us, we need to not
>>> crash, even if initially we thought NETIF_F_SG was available.
>>>
>>> Unless you want to hold RTNL in UDP xmit :)
>>>
>>> Ideally, GSO should be always on, as we did for TCP.
>>>
>>> Otherwise, I can guarantee syzkaller will hit again.
>>
>> Ah, right. Thanks, Eric!
>>
>> I'll read that feature bit only once.
>
> This issue is actually deeper and not specific to gso.
> With corking it is trivial to turn off sg in between calls.
>
> I'll need to send a separate fix for that.
This would do it. The extra branch is unfortunate, but I see no easy
way around it for the corking case.
It will obviously not build a linear skb, but validate_xmit_skb will clean
that up for such edge cases.
diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
index 66340ab750e6..e7daec7c7421 100644
--- a/net/ipv4/ip_output.c
+++ b/net/ipv4/ip_output.c
@@ -1040,7 +1040,8 @@ static int __ip_append_data(struct sock *sk,
if (copy > length)
copy = length;
- if (!(rt->dst.dev->features&NETIF_F_SG)) {
+ if (!(rt->dst.dev->features&NETIF_F_SG) &&
+ skb_tailroom(skb) >= copy) {
unsigned int off;
