Re: stmmac smatch error rx_queue_routing

[Niklas Cassel <niklas.cassel@axis.com>]

On Mon, Jan 22, 2018 at 04:55:10PM +0000, Jose Abreu wrote:
> Hi Niklas,

Hello Jose, and thanks for your reply.

> > Looking at the code raises some questions:
> >
> >
> > static void dwmac4_tx_queue_routing(struct mac_device_info *hw,
> >                                     u8 packet, u32 queue)
> > {
> >         ...
> >         static const struct stmmac_rx_routing route_possibilities[] = {
> >                 { GMAC_RXQCTRL_AVCPQ_MASK, GMAC_RXQCTRL_AVCPQ_SHIFT },
> >                 { GMAC_RXQCTRL_PTPQ_MASK, GMAC_RXQCTRL_PTPQ_SHIFT },
> >                 { GMAC_RXQCTRL_DCBCPQ_MASK, GMAC_RXQCTRL_DCBCPQ_SHIFT },
> >                 { GMAC_RXQCTRL_UPQ_MASK, GMAC_RXQCTRL_UPQ_SHIFT },
> >                 { GMAC_RXQCTRL_MCBCQ_MASK, GMAC_RXQCTRL_MCBCQ_SHIFT },
> >         };
> >
> >         value = readl(ioaddr + GMAC_RXQ_CTRL1);
> >
> >         /* routing configuration */
> >         value &= ~route_possibilities[packet - 1].reg_mask;
> >         value |= (queue << route_possibilities[packet-1].reg_shift) &
> >                  route_possibilities[packet - 1].reg_mask;
> >
> >
> > Calling the function with e.g. packet == 0 will lead to interesting stuff,
> > so the smatch warning is absolutely warranted.
> 
> Notice that stmmac_mac_config_rx_queues_routing() checks for
> packet == 0x0 and if its true then should never call
> rx_queue_routing, check [1] ...

It would probably be better if the check was in the function instead,
that way we don't have to hope that someone has audited all the places
where the function is called. Also, someone might add a call to that
function that doesn't do an explicit check before calling, and we
probably shouldn't overwrite random memory just because of that.

I also don't see any check that packet <= 6, anywhere,
which would also cause us to overwrite random memory.

> 
> >
> >
> > Looking where this function is used:
> >
> > static const struct stmmac_ops dwmac4_ops = {
> >         ...
> >         .rx_queue_routing = dwmac4_tx_queue_routing,
> >
> > Mixing rx and tx.. is this really correct?
> 
> It should be dwmac4_rx_queue_routing.

Ok. Feel free to submit a patch with my Reported-by if you wish.

> 
> >
> >
> >
> > Looking where the rx_queue_routing function is used:
> > git grep rx_queue_routing
> > stmmac_main.c:  if (rx_queues_count > 1 && priv->hw->mac->rx_queue_routing)
> >
> > it is just referenced in a single place, and we only check if function is
> > non-NULL, we never even call the function, so right now it is just unused
> > code.
> 
> [1] Another typo. You can see that in function
> stmmac_mac_config_rx_queues_routing() we are calling
> rx_queue_prio instead of rx_queue_routing ...

Ah, I see. Feel free to submit a patch that calls the correct function,
with my Reported-by, if you wish.


Regards,
Niklas