Re: [dpdk-dev] [PATCH v2] Fix `eventfd_link' module leakages and races

[Pavel Boldin <pboldin@mirantis.com>]

Hi Thomas,

There is by far more than 2 issues, but these are the only ones that
appeared to us.

The list of the issues that motivated the refactoring:

   1. Only one error code for every fault (-EFAULT).
   2. Copy-paste code from the `fget' function.
   3. Ambiguous variable names. The `files' variable mean different things
   under the same block. This ruins readability of the code.
   4. Overall placing of the all the code inside one `switch/case'. Modern
   kernel code style suggests placing these in the inline functions.
   5. Usage of the copy-pasted `close_fd' function code. (I really should
   use `sys_close' here though).


I can rewrite the refactoring in a set of small patches if this will aid
reviewers.

The dangerous bug that definitely must be fixed before the next release is
the `struct file*' leakage. I can provide a simple patch for this as a
separate submission and continue working on the refactoring patches. Is
this plan OK for you?


Pavel

On Mon, Mar 23, 2015 at 1:15 PM, Thomas Monjalon <thomas.monjalon@6wind.com>
wrote:

> Hi Pavel,
>
> You forgot the Signed-off-by line.
>
> Huawei, Changchun, any comment on this patch?
>
> 2015-03-18 15:16, Pavel Boldin:
> > The `eventfd_link' module provides an API to "steal" fd from another
> > process had been written with a bug that leaks `struct file' because
> > of the extra reference counter increment and missing `fput' call.
> >
> > The other bug is using another process' `task_struct' without
> incrementing
> > a reference counter.
>
> As there are 2 bugs, you should provide 2 patches.
>
> > Fix these bugs and refactor the module.
>
> Why refactoring along with the bug fixes?
> You'd have more chances to have your fixes integrated in 2.0 without
> refactoring.
>
> Thanks
>
>
>
>