Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Sat, 11 Apr 2020 15:42:51 -0300
From:   Marcelo Ricardo Leitner <mleitner@redhat.com>
To:     Pavel Machek <pavel@denx.de>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-kernel@vger.kernel.org, stable@vger.kernel.org,
        Qiujun Huang <hqjagain@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        syzbot+cea71eec5d6de256d54d@syzkaller.appspotmail.com
Subject: Re: [PATCH 4.19 03/54] sctp: fix refcount bug in sctp_wfree
Message-ID: <20200411184251.GM3625@localhost.localdomain>
References: <20200411115508.284500414@linuxfoundation.org>
 <20200411115508.593027768@linuxfoundation.org>
 <20200411182813.GA18221@duo.ucw.cz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200411182813.GA18221@duo.ucw.cz>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Sat, Apr 11, 2020 at 08:28:13PM +0200, Pavel Machek wrote:
> Hi!
> 
> > The following case cause the bug:
> > for the trouble SKB, it was in outq->transmitted list
> 
> Ok... but this is one hell of "interesting" code.
> 
> > --- a/net/sctp/socket.c
> > +++ b/net/sctp/socket.c
> > @@ -165,29 +165,44 @@ static void sctp_clear_owner_w(struct sc
> >  	skb_orphan(chunk->skb);
> >  }
> >  
> > +#define traverse_and_process()	\
> > +do {				\
> > +	msg = chunk->msg;	\
> > +	if (msg == prev_msg)	\
> > +		continue;	\
> > +	list_for_each_entry(c, &msg->chunks, frag_list) {	\
> > +		if ((clear && asoc->base.sk == c->skb->sk) ||	\
> > +		    (!clear && asoc->base.sk != c->skb->sk))	\
> > +			cb(c);	\
> > +	}			\
> > +	prev_msg = msg;		\
> > +} while (0)
> 
> Should this be function?
> 
> Do you see how it does "continue"? But the do {} while(0) wrapper eats
> the continue. "break" would be more clear here, but they are really
> equivalent due to way this macro is used.
> 
> It is just very, very confusing.

Agree. The 'continue' itself slipped in on a refactoring and I didn't
notice the confusing aspect of it. Initially, the code was written
without the macro, and the continue refererred to the outter
list_for_each_entry().

  Marcelo