From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f50.google.com (mail-wr1-f50.google.com [209.85.221.50]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 5CEA37082D for ; Wed, 20 May 2026 15:31:56 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.221.50 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1779291118; cv=none; b=cgEDTttiW2r2In4o9/7oPt4IFMggHaMGZmpnBYZ3aQHjzt/SU5pbHVUOSTPWr8nQ3sVJzvK4DrL9Kvypg+EQL7ufLsn/DgbCPrHOf4p6Z49Nf0E+aS2VG/m1CRv0rlTgU1idly15mFYG5wX1kXtm6aOLJ7B4YmjxGfyG0tFoT+Q= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1779291118; c=relaxed/simple; bh=LRRe5m4bSSb2oNxFZ3LnqP9xKzQo+YqDw1lAvlAqO6k=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=kRIUryO3mZOZzLb1yHwSw72jWgOzcA8M+dOkh80R5oYzr7krsXwParAQQcDBdFQCs3CL9fizK3buy7qeMcSQvazYc8UKoqMLigaqRgiRDXq7y+aM0UlMfFIacdxCAJCSsLPIwgLtqI0+FoyKSfywX2ZBFCH4Z9QvAfBIpT6lCho= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linaro.org; spf=pass smtp.mailfrom=linaro.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b=lu6Xo3Yv; arc=none smtp.client-ip=209.85.221.50 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linaro.org Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="lu6Xo3Yv" Received: by mail-wr1-f50.google.com with SMTP id ffacd0b85a97d-44e5624c053so3095516f8f.2 for ; Wed, 20 May 2026 08:31:56 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1779291115; x=1779895915; darn=vger.kernel.org; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=ZIImJajPM9fYtSPEthXg6jhil34ku8GvsFFkCo+lnSY=; b=lu6Xo3Yv3cBvgyI+Gn8PwIFRLfzHwCHZKPE/215PTHJdacghm6BZy3tJM8wS+F6NcG hKiGjUTUWpPwMhYkVwnWYwSuNzQsMUr0yDbLX7RpitBSzNbLZKOR4e85/GwL+YdvPFPL bJwRQx7ww9dcGE+wmT3M0EOULZSCDnuYCJuZcuFwzmxDREtwA6tB6uWAxzg82Euno6tv ZkqjYeDipkSXS20ijljIVZzq5WByDJeOgqk+FijE7GgJeoWDPsCnLATLrtSq0jfX0hN3 yj3SOuVAefpkDn7GgiLvCXa5Z0pgoGItnwlFeORCYwMPPCUL1/3FxTQ/vNztANei7iHa HkGw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1779291115; x=1779895915; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-gg:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=ZIImJajPM9fYtSPEthXg6jhil34ku8GvsFFkCo+lnSY=; b=f/SfUSAccPoYGMfWxyxou4UzKBmna2hVCM6+oHIZvFJgvosnhvOB8z6MtfUHWEQBSM sNVSY3KLGSwX+8yaEz/P3k9cJahaggf1Txtp7Z813ztZOccWSSWdpcUY2J1+fYNRAurU 0Q0DsgwAgUcZ8h3x3WJzUQuVHBcCZxdgIG1kW+m9yXUB3oPH8HrNZMxURE3gStIw3pmJ Wby3BtUy2rbzlKQYbtatNuMzlBaOdVXr3/Bfb9Q0ceOflAvfhnolWSNQYnTb3/0KQ734 8o1fiIsHqOQ9/bcP9s8F2SAFn2ByB/4cxlJnY7FUKLBQ94nlCBOsrYNp5kB+Ec4OOAtf wDOg== X-Forwarded-Encrypted: i=1; AFNElJ+DTuvkqj7WUcUovoGOKP/WbQUDgrjJS+L7ex6XDl/cvQAMThvzw5hondiXiVxME0ULhVI3rBdLqD4iA1g=@vger.kernel.org X-Gm-Message-State: AOJu0Yw0QCtdFs5tw87WXpYOwEpax8Q2qiDAK11kOuwdsQSjXKeuvtVX YIc3MprwxBwpPPE2l/XB9KhZzDb5HrWfpO/zvBSmjDDXm+28uuSqPHkm+Ao084B+e4c= X-Gm-Gg: Acq92OHBpR8pvwYcUom4P+v3pkOUStMIunCGaDW1J00lOl+OTipENV/T4VJC5CNCOWA jz74mpRqwujYKkI2Jnh9HHKTWkAOLFQEwpReqrjl3ForCTXVuZShWmYIVZNCbRxm4DxuWrnlGuq iWFCOgei7jbs7YlpcNDRILRN8ZfH2OXn+/YfckCkE7q5k36NAnLBJaN19TpcjHtfacQfaU7THre U6NhW/erMmU0bApkeNeLQ8174/YmIa5RHM5YNePkt6L3xQWTeZemOJ4GWmxVWisJFCJMnCSJ7pQ N7PP8xpJoLNAWXi7+hnrEkzD6/EF1CJpiSPwtNG/LweB2FjgrlIBqK6rTKEGXaCLBm3JUjvaAUg jX0I7pXmLg+tgIsyWTyOp2bj2oFADZckBxQHzkSHuZFl/7zPCEBsctdVcsNIVhvbKS1u8DrpnTF g0mvoCxce9bqQlYMxEjW5cKC6fNRiJ X-Received: by 2002:a05:6000:2508:b0:441:239e:2bb8 with SMTP id ffacd0b85a97d-45e5c58fe44mr38877967f8f.7.1779291114659; Wed, 20 May 2026 08:31:54 -0700 (PDT) Received: from [192.168.1.3] ([185.48.77.170]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-45d9ed2ffdfsm50958669f8f.15.2026.05.20.08.31.53 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 20 May 2026 08:31:54 -0700 (PDT) Message-ID: <463a9739-60c8-4f85-8293-b5a1197a2802@linaro.org> Date: Wed, 20 May 2026 16:31:53 +0100 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] coresight: fix resource leaks on path build failure To: Jie Gan , Mike Leach Cc: "coresight@lists.linaro.org" , "linux-arm-kernel@lists.infradead.org" , "linux-kernel@vger.kernel.org" , Suzuki Poulose , Leo Yan , Alexander Shishkin , Mathieu Poirier , Tingwei Zhang , Greg Kroah-Hartman , nd References: <20260513-fix-memory-leak-issue-v1-1-49822d7bc7d4@oss.qualcomm.com> <5b3785a1-0f53-4b61-bc18-880091c2ce12@oss.qualcomm.com> Content-Language: en-US From: James Clark In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit On 20/05/2026 11:13 am, Jie Gan wrote: > > > On 5/20/2026 5:27 PM, Mike Leach wrote: >> >> >>> -----Original Message----- >>> From: James Clark >>> Sent: Wednesday, May 20, 2026 9:38 AM >>> To: Jie Gan >>> Cc: coresight@lists.linaro.org; linux-arm-kernel@lists.infradead.org; >>> linux- >>> kernel@vger.kernel.org; Suzuki Poulose ; Mike >>> Leach ; Leo Yan ; Alexander >>> Shishkin ; Mathieu Poirier >>> ; Tingwei Zhang >>> ; Greg Kroah-Hartman >>> >>> Subject: Re: [PATCH] coresight: fix resource leaks on path build failure >>> >>> >>> >>> On 20/05/2026 2:55 am, Jie Gan wrote: >>>> >>>> >>>> On 5/19/2026 9:57 PM, James Clark wrote: >>>>> >>>>> >>>>> On 13/05/2026 2:32 am, Jie Gan wrote: >>>>>> Two related leaks when _coresight_build_path() encounters an error >>>>>> after >>>>>> coresight_grab_device() has already incremented the pm_runtime, >>> module, >>>>>> and device references for a node: >>>>>> >>>>>> 1. In _coresight_build_path(), if kzalloc_obj() for the path node >>>>>> fails >>>>>>      after coresight_grab_device() succeeds, >>>>>> coresight_drop_device() was >>>>>>      never called, permanently leaking all three references. >>>>>> >>>>>> 2. In coresight_build_path(), on failure the partial path was >>>>>> freed with >>>>>>      kfree(path) instead of coresight_release_path(path).  kfree() >>>>>> only >>>>>>      frees the coresight_path struct itself; it does not iterate >>>>>> path_list >>>>>>      to call coresight_drop_device() and kfree() for each >>>>>> coresight_node >>>>>>      already added by deeper recursive calls, leaking both the >>>>>> pm_runtime, >>>>>>      module, and device references and the node memory for every >>>>>> element >>>>>>      on the partial path. >>>>>> >>>>>> Fix both by adding coresight_drop_device() in the OOM unwind of >>>>>> _coresight_build_path(), and replacing kfree(path) with >>>>>> coresight_release_path(path) in coresight_build_path(). >>>>>> >>>>>> Fixes: 32b0707a4182 ("coresight: Add try_get_module() in >>>>>> coresight_grab_device()") >>>>>> Fixes: b3e94405941e ("coresight: associating path with session rather >>>>>> than tracer") >>>>>> Signed-off-by: Jie Gan >>>>>> --- >>>>>>    drivers/hwtracing/coresight/coresight-core.c | 6 ++++-- >>>>>>    1 file changed, 4 insertions(+), 2 deletions(-) >>>>>> >>>>>> diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/ >>>>>> hwtracing/coresight/coresight-core.c >>>>>> index 46f247f73cf6..c1354ea8e11d 100644 >>>>>> --- a/drivers/hwtracing/coresight/coresight-core.c >>>>>> +++ b/drivers/hwtracing/coresight/coresight-core.c >>>>>> @@ -825,8 +825,10 @@ static int _coresight_build_path(struct >>>>>> coresight_device *csdev, >>>>>>            return ret; >>>>>>        node = kzalloc_obj(struct coresight_node); >>>>>> -    if (!node) >>>>>> +    if (!node) { >>>>>> +        coresight_drop_device(csdev); >>>>>>            return -ENOMEM; >>>>>> +    } >>>>>>        node->csdev = csdev; >>>>>>        list_add(&node->link, &path->path_list); >>>>>> @@ -851,7 +853,7 @@ struct coresight_path >>>>>> *coresight_build_path(struct coresight_device *source, >>>>>>        rc = _coresight_build_path(source, source, sink, path); >>>>>>        if (rc) { >>>>>> -        kfree(path); >>>>>> +        coresight_release_path(path); >>>>>>            return ERR_PTR(rc); >>>>>>        } >>>>>> >>>>>> --- >>>>>> base-commit: e98d21c170b01ddef366f023bbfcf6b31509fa83 >>>>>> change-id: 20260513-fix-memory-leak-issue-034b4a45265e >>>>>> >>>>>> Best regards, >>>>> >>>>> Looks good to me, but sashiko is complaining: https://sashiko.dev/#/ >>>>> patchset/20260513-fix-memory-leak-issue- >>>>> v1-1-49822d7bc7d4%40oss.qualcomm.com >>>>> >>>>> I'm trying to understand why it's saying that, but I think the >>>>> scenario is that if there are multiple correct paths to a sink, when >>>>> one path partially fails and a second path succeeds you could get a >>>>> path_list with some garbage entries in it. >>>> >>>> I think the coresight_release_path is added to address this situation. >>>> We suffered the path partially failure, and we need release all nodes >>>> already added to the path. >>>> >>> >>> It wouldn't call coresight_release_path() in this case though. If one >>> path ends up building to success but a parallel path partially failed >>> then _coresight_build_path() still returns success. During the search it >>> would have still added the nodes from the partially failed path to the >>> path_list. This is only an issue if there are multiple correct paths. >>> > > The point here is there are multiple routes from the same source device > to the same sink device, am right? > > I have no experience on this scenario. So with the scenario, the > build_path may succeeded in one route and failed in another route, but > finally, the _coresight_build_path still returns success, is that correct? > >>>>> >>>>> That's kind of a different and existing issue to the one you've fixed, >>>>> and assumes that multiple paths to one sink are possible, which I'm >>>>> not sure is supported? >>>> >>>> Each path is unique. We only deal with the issue path for balancing the >>>> reference count. >>>> >>>> Thanks, >>>> Jie >>>> >>> >>> I'm not exactly sure what you mean by unique, but the same source and >>> sink could potentially be connected through two different sets of links. >>> >> >> Multiple paths between a source and sink are not permitted under the >> CoreSight spec. >> > > As Mike mentioned, my understanding is that a source device is only > allowed to be added to one valid path—this is what I mean by “unique.” > > Thanks, > Jie > That's ok then we can ignore this for this patch. But it would be good to enforce that in _coresight_build_path() with some kind of assert. Or at least add a comment to appease the AI reviewers. >> If such a system was to be built - then a fix would need to be in the >> declaration of connections - e.g. miss one path out in the device tree >> for example. Not up to the Coresight drivers to handle out of >> specification hardware. >> >> Mike >> >> >>>>> >>>>> It might be as easy as breaking the loop early for any return value >>>>> other than -ENODEV, but I'll leave it to you to decide whether to do >>>>> that here or not. >>>>> >>>>> Reviewed-by: James Clark >>>>> >>>> >> >