Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <286faa2529e01e6091666f97ad0cc703e5e80c7c.camel@redhat.com>
Subject: Re: [syzbot] WARNING in mptcp_sendmsg_frag
From:   Paolo Abeni <pabeni@redhat.com>
To:     Dan Carpenter <dan.carpenter@oracle.com>
Cc:     syzbot <syzbot+263a248eec3e875baa7b@syzkaller.appspotmail.com>,
        davem@davemloft.net, kuba@kernel.org, linux-kernel@vger.kernel.org,
        mathew.j.martineau@linux.intel.com, matthieu.baerts@tessares.net,
        mptcp@lists.linux.dev, netdev@vger.kernel.org,
        syzkaller-bugs@googlegroups.com
Date:   Thu, 23 Sep 2021 17:13:53 +0200
In-Reply-To: <20210923143728.GD2083@kadam>
References: <00000000000015991c05cc43a736@google.com>
         <7de92627f85522bf5640defe16eee6c8825f5c55.camel@redhat.com>
         <20210923141942.GD2048@kadam> <20210923143728.GD2083@kadam>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.36.5 (3.36.5-2.fc32) 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hello,

On Thu, 2021-09-23 at 17:37 +0300, Dan Carpenter wrote:
> On Thu, Sep 23, 2021 at 05:19:42PM +0300, Dan Carpenter wrote:
> > On Wed, Sep 22, 2021 at 12:32:56PM +0200, Paolo Abeni wrote:
> > > #syz test: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> > > 
> > > The debug code helped a bit. It looks like we have singed/unsigned
> > > comparisons issue
> > 
> > There should be a static checker warning for these.  I have created one
> > in response to your email.  It turns out there are a couple other
> > instances of this bug in the same file.

Thank you!

I was quite suprised the plain compiler did not emit a warn, even with
W=1.

> > net/mptcp/protocol.c:479 mptcp_subflow_could_cleanup() warn: unsigned subtraction: '(null)' use '!='
> 
> I should have checked my output a bit more carefully.  I don't want this
> one to generate a warning.
> 
> > net/mptcp/protocol.c:909 mptcp_frag_can_collapse_to() warn: unsigned subtraction: 'pfrag->size - pfrag->offset' use '!='
> 
> Likely "pfrag->offset" can't be larger than "pfrag->size".  Smatch has
> some code to try track this information but it's not clever enough.

Yes, this looks safe, offset can't be larger than size.

Even the last reported warning looks safe to me: 'info->size_goal -
skb->len', we just check for size_goal being greater then skb->len.

Cheers,

Paolo