* Re: sctp: Add GSO support
2016-06-06 20:16 sctp: Add GSO support Dan Carpenter
@ 2016-06-06 20:26 ` marcelo.leitner
2016-06-06 20:42 ` Dan Carpenter
2016-06-06 21:07 ` marcelo.leitner
2 siblings, 0 replies; 4+ messages in thread
From: marcelo.leitner @ 2016-06-06 20:26 UTC (permalink / raw)
To: linux-sctp
Hi Dan,
On Mon, Jun 06, 2016 at 11:16:46PM +0300, Dan Carpenter wrote:
> Hello Marcelo Ricardo Leitner,
>
> This is a semi-automatic email about new static checker warnings.
>
> The patch 90017accff61: "sctp: Add GSO support" from Jun 2, 2016,
> leads to the following Smatch complaint:
>
> net/sctp/output.c:122 sctp_packet_config()
> error: we previously assumed 'asoc' could be null (see line 94)
>
> net/sctp/output.c
> 93
> 94 if (asoc && tp->dst) {
> ^^^^
> New test.
>
> 95 struct sock *sk = asoc->base.sk;
> 96
> 97 rcu_read_lock();
> 98 if (__sk_dst_get(sk) != tp->dst) {
> 99 dst_hold(tp->dst);
> 100 sk_setup_caps(sk, tp->dst);
> 101 }
> 102
> 103 if (sk_can_gso(sk)) {
> 104 struct net_device *dev = tp->dst->dev;
> 105
> 106 packet->max_size = dev->gso_max_size;
> 107 } else {
> 108 packet->max_size = asoc->pathmtu;
> 109 }
> 110 rcu_read_unlock();
> 111
> 112 } else {
> 113 packet->max_size = tp->pathmtu;
> 114 }
> 115
> 116 if (ecn_capable && sctp_packet_empty(packet)) {
> 117 struct sctp_chunk *chunk;
> 118
> 119 /* If there a is a prepend chunk stick it on the list before
> 120 * any other chunks get appended.
> 121 */
> 122 chunk = sctp_get_ecne_prepend(asoc);
> ^^^^
> New unchecked dereference. It's possible that maybe checking
> ecn_capable and sctp_packet_empty() implies that "asoc" is non-NULL but
> it's not obvious from a glance. Anyway, just let me know if that's the
> case.
>
> 123 if (chunk)
> 124 sctp_packet_append_chunk(packet, chunk);
>
Thanks for the report.
It's a false-positive, I think. Is there a way that we can avoid the
warning without changing the code? I could add a check in there, like
'if (ecn_capable && asoc && ..' just to clear this but wouldn't like to
do it.
False-positive because ecn_capable cannot be true without an asoc, so
checking ecn_capable is enough to know that asoc is there too, as in:
$ git grep -A 1 sctp_packet_config
output.c:void sctp_packet_config(struct sctp_packet *packet, __u32 vtag,
output.c- int ecn_capable)
--
outqueue.c: sctp_packet_config(packet, vtag,
outqueue.c-
asoc->peer.ecn_capable);
--
outqueue.c: sctp_packet_config(&singleton, vtag, 0);
outqueue.c- sctp_packet_append_chunk(&singleton,
chunk);
--
outqueue.c: sctp_packet_config(packet, vtag,
outqueue.c-
asoc->peer.ecn_capable);
--
outqueue.c: sctp_packet_config(packet, vtag,
outqueue.c-
asoc->peer.ecn_capable);
--
sm_statefuns.c: sctp_packet_config(packet, vtag, 0);
sm_statefuns.c-
and in all these cases, asoc was verified already. Unless the tool can
do such check across the call stack and I missed something in it..
Regards,
Marcelo
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: sctp: Add GSO support
2016-06-06 20:16 sctp: Add GSO support Dan Carpenter
2016-06-06 20:26 ` marcelo.leitner
@ 2016-06-06 20:42 ` Dan Carpenter
2016-06-06 21:07 ` marcelo.leitner
2 siblings, 0 replies; 4+ messages in thread
From: Dan Carpenter @ 2016-06-06 20:42 UTC (permalink / raw)
To: linux-sctp
It's fairly tricky to follow actually. Here is one caller:
net/sctp/outqueue.c
706 static int sctp_outq_flush(struct sctp_outq *q, int rtx_timeout, gfp_t gfp)
707 {
708 struct sctp_packet *packet;
709 struct sctp_packet singleton;
710 struct sctp_association *asoc = q->asoc;
^^^^^^^^^^^^^^
asoc is set here.
711 __u16 sport = asoc->base.bind_addr.port;
712 __u16 dport = asoc->peer.port;
713 __u32 vtag = asoc->peer.i.init_tag;
714 struct sctp_transport *transport = NULL;
715 struct sctp_transport *new_transport;
716 struct sctp_chunk *chunk, *tmp;
717 sctp_xmit_t status;
[ snip ]
800
801 /* Are we switching transports?
802 * Take care of transport locks.
803 */
804 if (new_transport != transport) {
805 transport = new_transport;
806 if (list_empty(&transport->send_ready)) {
807 list_add_tail(&transport->send_ready,
808 &transport_list);
809 }
810 packet = &transport->packet;
^^^^^^^^^^^^^^^^^^^^^^^^^^^
packet is set here. How do we know that "packect->transport->asoc" and
"asoc" are the same? It's complicated.
811 sctp_packet_config(packet, vtag,
812 asoc->peer.ecn_capable);
813 }
Anyway, maybe eventually we'll figure out a way to make it work but even
just reading it manually is quite complicated....
For now, just ignore the false postive.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: sctp: Add GSO support
2016-06-06 20:16 sctp: Add GSO support Dan Carpenter
2016-06-06 20:26 ` marcelo.leitner
2016-06-06 20:42 ` Dan Carpenter
@ 2016-06-06 21:07 ` marcelo.leitner
2 siblings, 0 replies; 4+ messages in thread
From: marcelo.leitner @ 2016-06-06 21:07 UTC (permalink / raw)
To: linux-sctp
On Mon, Jun 06, 2016 at 11:42:38PM +0300, Dan Carpenter wrote:
> It's fairly tricky to follow actually. Here is one caller:
>
> net/sctp/outqueue.c
> 706 static int sctp_outq_flush(struct sctp_outq *q, int rtx_timeout, gfp_t gfp)
> 707 {
> 708 struct sctp_packet *packet;
> 709 struct sctp_packet singleton;
> 710 struct sctp_association *asoc = q->asoc;
> ^^^^^^^^^^^^^^
> asoc is set here.
>
> 711 __u16 sport = asoc->base.bind_addr.port;
> 712 __u16 dport = asoc->peer.port;
> 713 __u32 vtag = asoc->peer.i.init_tag;
> 714 struct sctp_transport *transport = NULL;
> 715 struct sctp_transport *new_transport;
> 716 struct sctp_chunk *chunk, *tmp;
> 717 sctp_xmit_t status;
>
> [ snip ]
>
> 800
> 801 /* Are we switching transports?
> 802 * Take care of transport locks.
> 803 */
> 804 if (new_transport != transport) {
> 805 transport = new_transport;
> 806 if (list_empty(&transport->send_ready)) {
> 807 list_add_tail(&transport->send_ready,
> 808 &transport_list);
> 809 }
> 810 packet = &transport->packet;
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^
> packet is set here. How do we know that "packect->transport->asoc" and
> "asoc" are the same? It's complicated.
Yes agreed, very tricky. It's somewhat redundant information, but that
makes life easier during different stages of its life cycle.
It's like: packets are created at sctp_outq_flush (and some other
places), and then sits on an outq which belongs to a given asoc, just
like the transport. But transports doesn't have queues yet that packet
created is arranged to be sent through a given transport.
And transports always belong to a single asoc, so it's two different
ways of reaching to the same info.
v
packet->transport->asoc = packet->transport->asoc.outq->asoc
^----------´
And to make it even more complicated, with have sctp_do_peeloff(), which
will take all packets belonging to a given asoc from one socket and move
to another, potentially updating asoc pointer during that.
> 811 sctp_packet_config(packet, vtag,
> 812 asoc->peer.ecn_capable);
> 813 }
>
> Anyway, maybe eventually we'll figure out a way to make it work but even
> just reading it manually is quite complicated....
>
> For now, just ignore the false postive.
Okay. Let me know if I can help any further.
Thanks,
Marcelo
^ permalink raw reply [flat|nested] 4+ messages in thread