Unsafe array accesses in rte_sched.c

Unsafe array accesses in rte_sched.c

[Simon Kågström]

Hi!

I'm investigating DPDK support for pacing output streams and trying to
understand the QoS framework. However, I quickly found some instances of
unsafe array accesses. E.g., the rte_sched_port_config_qsize function
looks like this:

  static void
  rte_sched_port_config_qsize(struct rte_sched_port *port)
  {
        /* TC 0 */
        port->qsize_add[0] = 0;
        port->qsize_add[1] = port->qsize_add[0] + port->qsize[0];
        port->qsize_add[2] = port->qsize_add[1] + port->qsize[0];
        port->qsize_add[3] = port->qsize_add[2] + port->qsize[0];

  [...]

        /* TC 3 */
        port->qsize_add[12] = port->qsize_add[11] + port->qsize[2];
        port->qsize_add[13] = port->qsize_add[12] + port->qsize[3];
        port->qsize_add[14] = port->qsize_add[13] + port->qsize[3];
        port->qsize_add[15] = port->qsize_add[14] + port->qsize[3];

        port->qsize_sum = port->qsize_add[15] + port->qsize[3];
  }

but port->qsize is actually defined as

  uint16_t qsize[RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE];

There are similar problems in rte_sched_port_log_pipe_profile() and
probably other places.


I don't understand the code well enough to send patches for these,
although the fixes should be fairly trivial. Perhaps this is already
known as it should be fairly easy to trigger with static checkers?

// Simon

Re: Unsafe array accesses in rte_sched.c

[Dumitrescu, Cristian]

> -----Original Message-----
> From: Simon Kågström [mailto:simon.kagstrom@netinsight.net]
> Sent: Friday, October 16, 2015 9:49 AM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; dev@dpdk.org
> Subject: Unsafe array accesses in rte_sched.c
> 
> Hi!
> 
> I'm investigating DPDK support for pacing output streams and trying to
> understand the QoS framework. However, I quickly found some instances of
> unsafe array accesses. E.g., the rte_sched_port_config_qsize function
> looks like this:
> 
>   static void
>   rte_sched_port_config_qsize(struct rte_sched_port *port)
>   {
>         /* TC 0 */
>         port->qsize_add[0] = 0;
>         port->qsize_add[1] = port->qsize_add[0] + port->qsize[0];
>         port->qsize_add[2] = port->qsize_add[1] + port->qsize[0];
>         port->qsize_add[3] = port->qsize_add[2] + port->qsize[0];
> 
>   [...]
> 
>         /* TC 3 */
>         port->qsize_add[12] = port->qsize_add[11] + port->qsize[2];
>         port->qsize_add[13] = port->qsize_add[12] + port->qsize[3];
>         port->qsize_add[14] = port->qsize_add[13] + port->qsize[3];
>         port->qsize_add[15] = port->qsize_add[14] + port->qsize[3];
> 
>         port->qsize_sum = port->qsize_add[15] + port->qsize[3];
>   }
> 
> but port->qsize is actually defined as
> 
>   uint16_t qsize[RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE];
> 

Not sure what you see "unsafe" here: qsize is an array of 4 elements, while qsize_add is a different array of 16 elements? Please explain.

> There are similar problems in rte_sched_port_log_pipe_profile() and
> probably other places.
> 
> 
> I don't understand the code well enough to send patches for these,
> although the fixes should be fairly trivial. Perhaps this is already
> known as it should be fairly easy to trigger with static checkers?
> 
> // Simon

Re: Unsafe array accesses in rte_sched.c

[Simon Kågström]

On 2015-10-16 15:39, Dumitrescu, Cristian wrote:
>>         port->qsize_add[12] = port->qsize_add[11] + port->qsize[2];
>>         port->qsize_add[13] = port->qsize_add[12] + port->qsize[3];
>>         port->qsize_add[14] = port->qsize_add[13] + port->qsize[3];
>>         port->qsize_add[15] = port->qsize_add[14] + port->qsize[3];
>>
>>         port->qsize_sum = port->qsize_add[15] + port->qsize[3];
>>   }
>>
>> but port->qsize is actually defined as
>>
>>   uint16_t qsize[RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE];
>>
> 
> Not sure what you see "unsafe" here: qsize is an array of 4 elements, while qsize_add is a different array of 16 elements? Please explain.

Sorry, I should have been more explicit: What I mean that the code
should loop over RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE instead of
hard-coding the numbers.

It certainly works with the current RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE,
but it would be safer (and in my opinion more clear) if it would not
assume RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE == 4.

// Simon

Re: Unsafe array accesses in rte_sched.c

[Stephen Hemminger]

On Fri, 16 Oct 2015 15:50:55 +0200
Simon Kågström <simon.kagstrom@netinsight.net> wrote:

> On 2015-10-16 15:39, Dumitrescu, Cristian wrote:
> >>         port->qsize_add[12] = port->qsize_add[11] + port->qsize[2];
> >>         port->qsize_add[13] = port->qsize_add[12] + port->qsize[3];
> >>         port->qsize_add[14] = port->qsize_add[13] + port->qsize[3];
> >>         port->qsize_add[15] = port->qsize_add[14] + port->qsize[3];
> >>
> >>         port->qsize_sum = port->qsize_add[15] + port->qsize[3];
> >>   }
> >>
> >> but port->qsize is actually defined as
> >>
> >>   uint16_t qsize[RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE];
> >>
> > 
> > Not sure what you see "unsafe" here: qsize is an array of 4 elements, while qsize_add is a different array of 16 elements? Please explain.
> 
> Sorry, I should have been more explicit: What I mean that the code
> should loop over RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE instead of
> hard-coding the numbers.
> 
> It certainly works with the current RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE,
> but it would be safer (and in my opinion more clear) if it would not
> assume RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE == 4.
> 
> // Simon

Unfortunately, current code has lots of hard wired assumptions about number
of traffic classes. Lots of manually loop unrolling etc. It would be better
if this was done using loops and telling compiler to unroll critical loops.