Re: [PATCH] Input: cros_ec_keyb - avoid variable-length arrays on stack

[Doug Anderson]

On Thu, Jan 2, 2014 at 4:25 PM, Luigi Semenzato <semenzato@xxxxxxxxxxxx> wrote:
> On Thu, Jan 2, 2014 at 11:48 AM, Dmitry Torokhov
> <dmitry.torokhov@xxxxxxxxx> wrote:
>> Hi Doug,
>>
>> On Thu, Jan 02, 2014 at 09:40:44AM -0800, Doug Anderson wrote:
>>> Dmitry,
>>>
>>> Thanks for cleaning up cros_eckeyb.  :)  I'm a little curious about
>>> the motivation here.  I can't imagine a keyboard with all that many
>>> columns (ours has 13), so it's really not taking a whole lot off of
>>> the stack.  Are you trying to make some sort of automated checker
>>> happy, or just generally trying to keep the kernel consistent?
>>
>> I compile most of the code with sparse so I prefer to keep it happy.
>>
>>>
>>> In any case, I'm not opposed to moving these bytes off the stack.
>>> Comments below, though...
>>>
>>> ---
>>>
>>> On Tue, Dec 31, 2013 at 11:35 AM, Dmitry Torokhov
>>> <dmitry.torokhov@xxxxxxxxx> wrote:
...
>>> > @@ -217,32 +219,40 @@ static int cros_ec_keyb_probe(struct platform_device *pdev)
>>> >         struct cros_ec_keyb *ckdev;
>>> >         struct input_dev *idev;
>>> >         struct device_node *np;
>>> > +       unsigned int rows, cols;
>>> > +       size_t size;
>>> >         int err;
>>> >
>>> >         np = pdev->dev.of_node;
>>> >         if (!np)
>>> >                 return -ENODEV;
>>> >
>>> > -       ckdev = devm_kzalloc(&pdev->dev, sizeof(*ckdev), GFP_KERNEL);
>>> > -       if (!ckdev)
>>> > -               return -ENOMEM;
>>> > -       err = matrix_keypad_parse_of_params(&pdev->dev, &ckdev->rows,
>>> > -                                           &ckdev->cols);
>>> > +       err = matrix_keypad_parse_of_params(&pdev->dev, &rows, &cols);
>>> >         if (err)
>>> >                 return err;
>>> > -       ckdev->old_kb_state = devm_kzalloc(&pdev->dev, ckdev->cols, GFP_KERNEL);
>>> > -       if (!ckdev->old_kb_state)
>>> > -               return -ENOMEM;
>>> >
>>> > -       idev = devm_input_allocate_device(&pdev->dev);
>>> > -       if (!idev)
>>> > +       /*
>>> > +        * Double memory for keyboard state so we have space for storing
>>> > +        * current and previous state.
>>> > +        */
>>> > +       size = sizeof(*ckdev) + 2 * cols * sizeof(*ckdev->kb_state);
>>> > +       ckdev = devm_kzalloc(&pdev->dev, size, GFP_KERNEL);
>>> > +       if (!ckdev)
>>> >                 return -ENOMEM;
>>>
>>> This change seems like a lot of complexity to save one memory
>>> allocation.  If you insist, I'd be OK with having one allocation for
>>> both buffers (kb_state and old_kb_state) but trying to jam this onto
>>> the end of the structure is non-obvious.  It certainly took me a
>>> minute to understand what you were doing and why.
>>
>> It is not one additional allocation but more as you need to allocate
>> devres data structures and add them there. I think we have quite a few
>> drivers piggy-backing key tables at the end of data structures.

OK, I will leave this as your call.  To me, piggybacking like this
make sense if you've got a single chunk of dynamic memory that you
just want to cram onto the end of the structure.  It just gets more
complicated when you have two nearly identical chunks of memory and
one of them is using this piggybacking technique while the other
isn't.

What about a compromise and declaring as:

 u8 *kb_state;
 u8 *old_kb_state;
 u8 buffers[];

You still have the same number of memory allocations but (to me) it's
much clearer what's going on here.  You do pay a penalty of an extra
memory dereference and an extra 4 bytes of memory, but clarity should
trump that.

-Doug
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/