From mboxrd@z Thu Jan 1 00:00:00 1970 From: Pavel Boldin Subject: Re: [PATCH v2] Fix `eventfd_link' module leakages and races Date: Mon, 23 Mar 2015 13:36:46 +0200 Message-ID: References: <1426524059-30886-1-git-send-email-pboldin+dpdk@mirantis.com> <1426684571-14782-1-git-send-email-pboldin@mirantis.com> <2544479.OZtlEpgWBm@xps13> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Cc: "dev-VfR2kkLFssw@public.gmane.org" To: Thomas Monjalon Return-path: In-Reply-To: <2544479.OZtlEpgWBm@xps13> List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces-VfR2kkLFssw@public.gmane.org Sender: "dev" 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 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 > > > >