* re: net: sctp: rework multihoming retransmission path selection to rfc4960
@ 2014-02-28 23:15 Dan Carpenter
2014-02-28 23:30 ` Daniel Borkmann
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Dan Carpenter @ 2014-02-28 23:15 UTC (permalink / raw)
To: linux-sctp
Hello Daniel Borkmann,
This is a semi-automatic email about new static checker warnings.
The patch 4c47af4d5eb2: "net: sctp: rework multihoming retransmission
path selection to rfc4960" from Feb 20, 2014, leads to the following
Smatch complaint:
net/sctp/associola.c:1322 sctp_assoc_update_retran_path()
warn: variable dereferenced before check 'trans_next' (see line 1319)
net/sctp/associola.c
1305 /* Iterate from retran_path's successor back to retran_path. */
1306 for (trans = list_next_entry(trans, transports); 1;
1307 trans = list_next_entry(trans, transports)) {
1308 /* Manually skip the head element. */
1309 if (&trans->transports = &asoc->peer.transport_addr_list)
1310 continue;
1311 if (trans->state = SCTP_UNCONFIRMED)
1312 continue;
1313 trans_next = sctp_trans_elect_best(trans, trans_next);
1314 /* Active is good enough for immediate return. */
1315 if (trans_next->state = SCTP_ACTIVE)
^^^^^^^^^^^^^^^^^
Dereference.
1316 break;
^^^^^^
1317 /* We've reached the end, time to update path. */
1318 if (trans = asoc->peer.retran_path)
1319 break;
^^^^^
These two breaks are the the only way to exit from the loop.
1320 }
1321
1322 if (trans_next != NULL)
^^^^^^^^^^^^^^^^^^
Check to late.
1323 asoc->peer.retran_path = trans_next;
1324
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: net: sctp: rework multihoming retransmission path selection to rfc4960
2014-02-28 23:15 Dan Carpenter
@ 2014-02-28 23:30 ` Daniel Borkmann
2014-03-01 8:44 ` Dan Carpenter
2014-03-01 10:24 ` Daniel Borkmann
2 siblings, 0 replies; 6+ messages in thread
From: Daniel Borkmann @ 2014-02-28 23:30 UTC (permalink / raw)
To: linux-sctp
On 03/01/2014 12:15 AM, Dan Carpenter wrote:
> Hello Daniel Borkmann,
>
> This is a semi-automatic email about new static checker warnings.
>
> The patch 4c47af4d5eb2: "net: sctp: rework multihoming retransmission
> path selection to rfc4960" from Feb 20, 2014, leads to the following
> Smatch complaint:
>
> net/sctp/associola.c:1322 sctp_assoc_update_retran_path()
> warn: variable dereferenced before check 'trans_next' (see line 1319)
>
> net/sctp/associola.c
> 1305 /* Iterate from retran_path's successor back to retran_path. */
> 1306 for (trans = list_next_entry(trans, transports); 1;
> 1307 trans = list_next_entry(trans, transports)) {
> 1308 /* Manually skip the head element. */
> 1309 if (&trans->transports = &asoc->peer.transport_addr_list)
> 1310 continue;
> 1311 if (trans->state = SCTP_UNCONFIRMED)
> 1312 continue;
> 1313 trans_next = sctp_trans_elect_best(trans, trans_next);
> 1314 /* Active is good enough for immediate return. */
> 1315 if (trans_next->state = SCTP_ACTIVE)
> ^^^^^^^^^^^^^^^^^
> Dereference.
That is a false-positive.
trans_next at that time is being assigned through sctp_trans_elect_best() a
guaranteed non-NULL pointer.
> 1316 break;
> ^^^^^^
> 1317 /* We've reached the end, time to update path. */
> 1318 if (trans = asoc->peer.retran_path)
> 1319 break;
> ^^^^^
> These two breaks are the the only way to exit from the loop.
>
> 1320 }
> 1321
> 1322 if (trans_next != NULL)
> ^^^^^^^^^^^^^^^^^^
> Check to late.
>
> 1323 asoc->peer.retran_path = trans_next;
> 1324
>
> regards,
> dan carpenter
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: net: sctp: rework multihoming retransmission path selection to rfc4960
2014-02-28 23:15 Dan Carpenter
2014-02-28 23:30 ` Daniel Borkmann
@ 2014-03-01 8:44 ` Dan Carpenter
2014-03-01 10:24 ` Daniel Borkmann
2 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2014-03-01 8:44 UTC (permalink / raw)
To: linux-sctp
On Sat, Mar 01, 2014 at 12:30:09AM +0100, Daniel Borkmann wrote:
> On 03/01/2014 12:15 AM, Dan Carpenter wrote:
> >Hello Daniel Borkmann,
> >
> >This is a semi-automatic email about new static checker warnings.
> >
> >The patch 4c47af4d5eb2: "net: sctp: rework multihoming retransmission
> >path selection to rfc4960" from Feb 20, 2014, leads to the following
> >Smatch complaint:
> >
> >net/sctp/associola.c:1322 sctp_assoc_update_retran_path()
> > warn: variable dereferenced before check 'trans_next' (see line 1319)
> >
> >net/sctp/associola.c
> > 1305 /* Iterate from retran_path's successor back to retran_path. */
> > 1306 for (trans = list_next_entry(trans, transports); 1;
> > 1307 trans = list_next_entry(trans, transports)) {
> > 1308 /* Manually skip the head element. */
> > 1309 if (&trans->transports = &asoc->peer.transport_addr_list)
> > 1310 continue;
> > 1311 if (trans->state = SCTP_UNCONFIRMED)
> > 1312 continue;
> > 1313 trans_next = sctp_trans_elect_best(trans, trans_next);
> > 1314 /* Active is good enough for immediate return. */
> > 1315 if (trans_next->state = SCTP_ACTIVE)
> > ^^^^^^^^^^^^^^^^^
> >Dereference.
>
> That is a false-positive.
>
> trans_next at that time is being assigned through sctp_trans_elect_best() a
> guaranteed non-NULL pointer.
>
Can you remove the NULL check then?
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: net: sctp: rework multihoming retransmission path selection to rfc4960
2014-02-28 23:15 Dan Carpenter
2014-02-28 23:30 ` Daniel Borkmann
2014-03-01 8:44 ` Dan Carpenter
@ 2014-03-01 10:24 ` Daniel Borkmann
2 siblings, 0 replies; 6+ messages in thread
From: Daniel Borkmann @ 2014-03-01 10:24 UTC (permalink / raw)
To: linux-sctp
On 03/01/2014 09:44 AM, Dan Carpenter wrote:
> On Sat, Mar 01, 2014 at 12:30:09AM +0100, Daniel Borkmann wrote:
>> On 03/01/2014 12:15 AM, Dan Carpenter wrote:
>>> Hello Daniel Borkmann,
>>>
>>> This is a semi-automatic email about new static checker warnings.
>>>
>>> The patch 4c47af4d5eb2: "net: sctp: rework multihoming retransmission
>>> path selection to rfc4960" from Feb 20, 2014, leads to the following
>>> Smatch complaint:
>>>
>>> net/sctp/associola.c:1322 sctp_assoc_update_retran_path()
>>> warn: variable dereferenced before check 'trans_next' (see line 1319)
>>>
>>> net/sctp/associola.c
>>> 1305 /* Iterate from retran_path's successor back to retran_path. */
>>> 1306 for (trans = list_next_entry(trans, transports); 1;
>>> 1307 trans = list_next_entry(trans, transports)) {
>>> 1308 /* Manually skip the head element. */
>>> 1309 if (&trans->transports = &asoc->peer.transport_addr_list)
>>> 1310 continue;
>>> 1311 if (trans->state = SCTP_UNCONFIRMED)
>>> 1312 continue;
>>> 1313 trans_next = sctp_trans_elect_best(trans, trans_next);
>>> 1314 /* Active is good enough for immediate return. */
>>> 1315 if (trans_next->state = SCTP_ACTIVE)
>>> ^^^^^^^^^^^^^^^^^
>>> Dereference.
>>
>> That is a false-positive.
>>
>> trans_next at that time is being assigned through sctp_trans_elect_best() a
>> guaranteed non-NULL pointer.
>
> Can you remove the NULL check then?
Will do when this merges into net-next. Thanks.
> regards,
> dan carpenter
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: net: sctp: rework multihoming retransmission path selection to rfc4960
[not found] <20140305192635.6D9F7660D13@gitolite.kernel.org>
@ 2014-03-06 6:02 ` Dave Jones
2014-03-06 9:42 ` Daniel Borkmann
0 siblings, 1 reply; 6+ messages in thread
From: Dave Jones @ 2014-03-06 6:02 UTC (permalink / raw)
To: netdev; +Cc: dborkman, linux-coverity
On Wed, Mar 05, 2014 at 07:26:35PM +0000, Linux Kernel wrote:
> Gitweb: http://git.kernel.org/linus/;a=commit;h=4c47af4d5eb2c2f78f886079a3920a7078a6f0a0
> Commit: 4c47af4d5eb2c2f78f886079a3920a7078a6f0a0
> Parent: b194c1f1dbd5f2671e49e0ac801b1b78dc7de93b
> Author: Daniel Borkmann <dborkman@redhat.com>
> AuthorDate: Thu Feb 20 20:51:06 2014 +0100
> Committer: David S. Miller <davem@davemloft.net>
> CommitDate: Sat Feb 22 00:26:05 2014 -0500
>
> net: sctp: rework multihoming retransmission path selection to rfc4960
minor nit that coverity just picked up..
> + /* Iterate from retran_path's successor back to retran_path. */
> + for (trans = list_next_entry(trans, transports); 1;
> + trans = list_next_entry(trans, transports)) {
> + /* Manually skip the head element. */
> + if (&trans->transports == &asoc->peer.transport_addr_list)
> + continue;
> + if (trans->state == SCTP_UNCONFIRMED)
> + continue;
> + trans_next = sctp_trans_elect_best(trans, trans_next);
> + /* Active is good enough for immediate return. */
> + if (trans_next->state == SCTP_ACTIVE)
> break;
^ trans_next dereference ...
> + /* We've reached the end, time to update path. */
> + if (trans == asoc->peer.retran_path)
> break;
> }
>
> + if (trans_next != NULL)
> + asoc->peer.retran_path = trans_next;
Belated NULL check.
Either the check is redundant, or it needs to be moved up.
Dave
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: net: sctp: rework multihoming retransmission path selection to rfc4960
2014-03-06 6:02 ` net: sctp: rework multihoming retransmission path selection to rfc4960 Dave Jones
@ 2014-03-06 9:42 ` Daniel Borkmann
0 siblings, 0 replies; 6+ messages in thread
From: Daniel Borkmann @ 2014-03-06 9:42 UTC (permalink / raw)
To: Dave Jones; +Cc: netdev, linux-coverity
On 03/06/2014 07:02 AM, Dave Jones wrote:
...
> Either the check is redundant, or it needs to be moved up.
Yep, first one.
I already have a patch in my local tree, will send it out
some time this week.
Cheers,
Daniel
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-03-06 9:42 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20140305192635.6D9F7660D13@gitolite.kernel.org>
2014-03-06 6:02 ` net: sctp: rework multihoming retransmission path selection to rfc4960 Dave Jones
2014-03-06 9:42 ` Daniel Borkmann
2014-02-28 23:15 Dan Carpenter
2014-02-28 23:30 ` Daniel Borkmann
2014-03-01 8:44 ` Dan Carpenter
2014-03-01 10:24 ` Daniel Borkmann
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.