sctp: Add GSO support

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);

regards,
dan carpenter

Hi Dan,

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

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

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.

Okay. Let me know if I can help any further.

Thanks,
Marcelo