From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1751277AbeAURyL (ORCPT <rfc822;w@1wt.eu>);
        Sun, 21 Jan 2018 12:54:11 -0500
Received: from outprodmail01.cc.columbia.edu ([128.59.72.39]:42798 "EHLO
        outprodmail01.cc.columbia.edu" rhost-flags-OK-OK-OK-OK)
        by vger.kernel.org with ESMTP id S1750885AbeAURyJ (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Sun, 21 Jan 2018 12:54:09 -0500
X-Google-Smtp-Source: AH8x225q9agayJ5ortrFPqjfe7TI/xmRvoNfQ12g25MsPblWabR/+YSumw+B89euKEvRl3zVauUS4AbT+4212MmOT9A=
MIME-Version: 1.0
In-Reply-To: <CAASgV=soGj5DNBesK90kjqbUh60TWSwzKF5pj++oAnzW9hVPZQ@mail.gmail.com>
References: <CAASgV=tW5gDF8-bq_t+LYNaoLOnCbL94_u8+uF=s36-JNaGP_w@mail.gmail.com>
 <CACT4Y+ZcbKWkS5Zd6uAynL=PKF7KbTBWrxnkkPXb0spV0-EDsg@mail.gmail.com>
 <CAASgV=uWyvLTdObEMO0oPGzeMuvTfOTssHKVbBkYNcocrWioPA@mail.gmail.com>
 <CACT4Y+bs8xx-j0Uu_LKbcg2G1_U3LuTdxtg1wNji+7QFzJWxdA@mail.gmail.com> <CAASgV=soGj5DNBesK90kjqbUh60TWSwzKF5pj++oAnzW9hVPZQ@mail.gmail.com>
From: Shankara Pailoor <sp3485@columbia.edu>
Date: Sun, 21 Jan 2018 09:54:00 -0800
Message-ID: <CAASgV=uEXBNdAQE2==1_9R7FG0wQPfTqTJuE4_QqTBV18T5pQQ@mail.gmail.com>
Subject: Re: Possible Memory Leak in KCOV Linux 4.15-rc1
To: Dmitry Vyukov <dvyukov@google.com>
Cc: LKML <linux-kernel@vger.kernel.org>,
        syzkaller <syzkaller@googlegroups.com>,
        Andrew Zhu Aday <andrew.aday@columbia.edu>
Content-Type: text/plain; charset="UTF-8"
X-No-Spam-Score: Local
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Below is a reproducer.

#define _GNU_SOURCE
#include <fcntl.h>
#include <sys/ioctl.h>
#include <sys/mman.h>
#include <stdlib.h>
#include <unistd.h>

#define KCOV_INIT_TRACE _IOR('c', 1, unsigned long)
#define KCOV_ENABLE _IO('c', 100)
#define KCOV_DISABLE _IO('c', 101)
#define COVER_SIZE (16 << 20)


void kcov_setup() {
    unsigned long *cover;
    int fd;

    fd = open("/sys/kernel/debug/kcov", O_RDWR);
    if (fd == -1)
        perror("open"), exit(1);
    if (ioctl(fd, KCOV_INIT_TRACE, COVER_SIZE))
        perror("ioctl"), exit(1);
    cover = (unsigned long*)mmap(NULL,
                                COVER_SIZE * sizeof(unsigned long),
                                PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
    if ((void*)cover == MAP_FAILED)
        perror("mmap"), exit(1);
    if (ioctl(fd, KCOV_ENABLE, 0))
        perror("ioctl"), exit(1);

}

void main() {
    int i;
    for (i = 0; i < 4; i++)
        kcov_setup();
    sleep(10);
}

On Sun, Jan 21, 2018 at 1:11 AM, Shankara Pailoor <sp3485@columbia.edu> wrote:
> Hi Dmitry,
>
> The leaks went away when I disabled and closed the old file
> descriptors before opening new ones.
>
> The patch you sent wouldn't work because t is not initialized at the
> line. This seems to work for me
>
> diff --git a/kernel/kcov.c b/kernel/kcov.c
> index 7594c03..1397006 100644
> --- a/kernel/kcov.c
> +++ b/kernel/kcov.c
> @@ -371,6 +371,8 @@ static int kcov_ioctl_locked(struct kcov *kcov,
> unsigned int cmd,
>                 else
>                         return -EINVAL;
>                 t = current;
> +              if (!t->kcov)
> +                       return -EBUSY;
>                 /* Cache in task struct for performance. */
>                 t->kcov_size = kcov->size;
>                 t->kcov_area = kcov->area;
>
> On Sat, Jan 20, 2018 at 7:06 AM, Dmitry Vyukov <dvyukov@google.com> wrote:
>> On Sat, Jan 20, 2018 at 4:01 PM, Shankara Pailoor <sp3485@columbia.edu> wrote:
>>> Hi Dmitry,
>>>
>>> I will try and get something to you tomorrow. Just wondering, but what
>>> happens to the old struct kcov if a task opens /sys/kernel/debug/kcov
>>> twice? I am looking here
>>> https://elixir.free-electrons.com/linux/v4.15-rc8/source/kernel/kcov.c#L381
>>> and I don't see where the previous struct would get freed.
>>
>> Good question. Perhaps we need something like:
>>
>> diff --git a/kernel/kcov.c b/kernel/kcov.c
>> index 7594c033d98a..c76498018500 100644
>> --- a/kernel/kcov.c
>> +++ b/kernel/kcov.c
>> @@ -358,7 +358,7 @@ static int kcov_ioctl_locked(struct kcov *kcov,
>> unsigned int cmd,
>>                  */
>>                 if (kcov->mode != KCOV_MODE_INIT || !kcov->area)
>>                         return -EINVAL;
>> -               if (kcov->t != NULL)
>> +               if (kcov->t != NULL || t->kcov != NULL)
>>                         return -EBUSY;
>>                 if (arg == KCOV_TRACE_PC)
>>                         kcov->mode = KCOV_MODE_TRACE_PC;
>>
>>
>>
>>> On Sat, Jan 20, 2018 at 4:38 AM, Dmitry Vyukov <dvyukov@google.com> wrote:
>>>> On Fri, Jan 19, 2018 at 8:29 PM, Shankara Pailoor <sp3485@columbia.edu> wrote:
>>>>> Hi Dmitry,
>>>>>
>>>>> I added support for kcov in strace and I have been tracing a fairly
>>>>> large program but after a little while, I notice that when I mmap a
>>>>> new cover buffer, the call fails with ENOMEM. After killing the
>>>>> program, I try and rerun and I notice that there is nearly no memory
>>>>> on the system. When I do a kmemleak scan I get the following reports:
>>>>>
>>>>> I believe the problem occurs when I try and setup the kcov buffer
>>>>> again after an exec. Instead of reusing the old file descriptor I open
>>>>> kcov again within that process. In that case, I don't know what
>>>>> happens to the old kcov struct.
>>>>>
>>>>> I don't see a maintainers list for kcov so I decided to email you
>>>>> directly. Let me know what more information I can provide.
>>>>
>>>>
>>>> Hi Shankara,
>>>>
>>>> Looks bad. Can you provide a reproducer?
>>>> We extensively use kcov with syzkaller, but have not observed such
>>>> leaks. Also I don't see anything obvious in the code.
>>>>
>>>> Thanks
>>>