re: rt2x00: Fix queue related oops in case of deselected mac80211 multi-queue feature.

re: rt2x00: Fix queue related oops in case of deselected mac80211 multi-queue feature.

[Dan Carpenter]

Hello Gertjan van Wingerde,

I have a question about patch 61448f88078e: "rt2x00: Fix queue related
oops in case of deselected mac80211 multi-queue feature." from May 10,
2008 because I think there is an off by one.

drivers/net/wireless/ralink/rt2x00/rt2x00queue.c
  1239          /*
  1240           * We need the following queues:
  1241           * RX: 1
  1242           * TX: ops->tx_queues
  1243           * Beacon: 1
  1244           * Atim: 1 (if required)
  1245           */
  1246          rt2x00dev->data_queues = 2 + rt2x00dev->ops->tx_queues + req_atim;
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
We allocate everything at once in once chunk of memory.

  1247  
  1248          queue = kcalloc(rt2x00dev->data_queues, sizeof(*queue), GFP_KERNEL);
  1249          if (!queue) {
  1250                  rt2x00_err(rt2x00dev, "Queue allocation failed\n");
  1251                  return -ENOMEM;
  1252          }
  1253  
  1254          /*
  1255           * Initialize pointers
  1256           */
  1257          rt2x00dev->rx = queue;

This is equivalent to &queue[0].  It's actually helpful to static
checkers and people reading the code if you write it that because we
are talking about the first element only and not the whole buffer.
Meanwhile, people do it the reverse way and refer to &foo->start to talk
about that whole "foo" buffer...  :/

  1258          rt2x00dev->tx = &queue[1];
  1259          rt2x00dev->bcn = &queue[1 + rt2x00dev->ops->tx_queues];

There are 2 ->tx_queues, I think so we skipped one queue.  We should
have put it at &queue[2].  I looked at it briefly and I didn't see where
the second queue is ever used so maybe this is harmless beyond the
slight waste of memory.

  1260          rt2x00dev->atim = req_atim ? &queue[2 + rt2x00dev->ops->tx_queues] : NULL;
  1261  
  1262          /*

regards,
dan carpenter

re: rt2x00: Fix queue related oops in case of deselected mac80211 multi-queue feature.

[Gertjan van Wingerde]

Hi Dan,

(resending due to linux-wireless not accepting my message).

On Mon, Feb 8, 2016 at 6:55 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> Hello Gertjan van Wingerde,
>
> I have a question about patch 61448f88078e: "rt2x00: Fix queue related
> oops in case of deselected mac80211 multi-queue feature." from May 10,
> 2008 because I think there is an off by one.

Ouch, that's really a long time ago ;-)
I haven't been involved in rt2x00 for quite some time now, so let's see if I can
remember anything about this.

> drivers/net/wireless/ralink/rt2x00/rt2x00queue.c
>   1239          /*
>   1240           * We need the following queues:
>   1241           * RX: 1
>   1242           * TX: ops->tx_queues
>   1243           * Beacon: 1
>   1244           * Atim: 1 (if required)
>   1245           */
>   1246          rt2x00dev->data_queues = 2 + rt2x00dev->ops->tx_queues + req_atim;
>                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> We allocate everything at once in once chunk of memory.


Correct. That just simplifies the error handling and the whole piece of code.

The comment is the important thing here. These are the number of queues we have
to allocate. ops->tx_queues denotes the number of TX queues that the
hardware has.
As far as I can remember there are Ralink devices with 2 TX queues and
there are Ralink
devices with 4 TX queues. Also, there were devices with an ATIM queue
and there were
devices without an ATIM queue (again hardware dependent).

>   1247
>   1248          queue = kcalloc(rt2x00dev->data_queues, sizeof(*queue), GFP_KERNEL);
>   1249          if (!queue) {
>   1250                  rt2x00_err(rt2x00dev, "Queue allocation failed\n");
>   1251                  return -ENOMEM;
>   1252          }
>   1253
>   1254          /*
>   1255           * Initialize pointers
>   1256           */
>   1257          rt2x00dev->rx = queue;
>
> This is equivalent to &queue[0].  It's actually helpful to static
> checkers and people reading the code if you write it that because we
> are talking about the first element only and not the whole buffer.
> Meanwhile, people do it the reverse way and refer to &foo->start to talk
> about that whole "foo" buffer...  :/


I guess I can agree with that. Feel free to send a patch to "fix" this.

>   1258          rt2x00dev->tx = &queue[1];
>   1259          rt2x00dev->bcn = &queue[1 + rt2x00dev->ops->tx_queues];
>
> There are 2 ->tx_queues, I think so we skipped one queue.  We should
> have put it at &queue[2].  I looked at it briefly and I didn't see where
> the second queue is ever used so maybe this is harmless beyond the
> slight waste of memory.


As I mentioned above there can be either 2 or 4 TX queues. So for the
example of 2 TX queues,
these will be:
    &queue[1]
    &queue[2]

The beacon queue will be the next one, so this one will have to be
&queue[3], which is exactly
1 + number of TX queues (= 2 in this example).

Note that rt2x00dev->tx is supposed to be array of TX queues, not just
a single queue.

>
>
>   1260          rt2x00dev->atim = req_atim ? &queue[2 + rt2x00dev->ops->tx_queues] : NULL;
>   1261
>   1262          /*
>

Next, the ATIM queue is there (when present on the hardware).
Continuing the example of
above, this should be &queue[4], which again is exactly 2 + number of TX queues.

So, looking at this, I don't think there is an off-by-one error here.

---
Gertjan

-- 
---
Gertjan

Re: rt2x00: Fix queue related oops in case of deselected mac80211 multi-queue feature.

[Dan Carpenter]

Ah.  Right I was expecting the order to be different, but the comment
does explain it.  Thanks for looking at this.

regards,
dan carpenter