From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f41.google.com (mail-wr1-f41.google.com [209.85.221.41]) (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 22DF11C84D7 for ; Thu, 27 Nov 2025 06:35:29 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.221.41 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1764225332; cv=none; b=Gsy6bffwRjQVjIUmByj1xJQ/n7zggxywsDfRxWZRrHN3E9hf/+SSuA4zD/B8ViSCNRPKKdHH2/m4YkHFSs+CsZJ0FwJa788MhGDNUbTn5s7It+ulrzTuVqu6neG6P/F2j+xTbSykG9qblkt6setkDsntvDn9IWGNh6rkU/sUbDo= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1764225332; c=relaxed/simple; bh=K7wkWLwbfTcY/AfS2EVsUuILQbKr9hsQgn/zdqfz6PE=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=VrqYv/Kp0h3u8HQtY8U1Dwen8dF3LXiFw/ieabD8/WsuhxWUE/ZTMU3MyIFQMbbQxIF19m+UvI0FeCGWkBt0JrPBTxAxRiBPlESxFNSeOjOjWhQ990GB9gOyHVp45gAliXioslqSDtXJKsZTxZNguANnvK1DD5Tsx11k5uSlhLA= 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=nLzFJ7Q9; arc=none smtp.client-ip=209.85.221.41 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="nLzFJ7Q9" Received: by mail-wr1-f41.google.com with SMTP id ffacd0b85a97d-42bb288c17bso294041f8f.2 for ; Wed, 26 Nov 2025 22:35:29 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1764225328; x=1764830128; darn=vger.kernel.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=QG1ab1vJmGEim6W8b8KLS1C7JN6Szi3cb3yM3drSkzs=; b=nLzFJ7Q9KyDnAvTkBXdCh2SXI8uH2QS8kNnCtbA3tTYoa2B/swOOzneO0iFRM6OaXj DG5nMaJEXZiXoOvbuo6mxsCa4bddZv3bNVUDu14xXFDhXZtYfLsHS9wvJj5O0E54ECea hgJm+rGhEiOX0HtvSkomOgCSC7d4Q/Ubphpiu2rWlK60v+Y0dzqds4R9i7EBK04VRM/x Fw94thfTRPHwztVPU8KtYn3TWSDUuvoBhzaEyAWO3jnqWnfgbhptXgf2dNMFG17pzGgw BLpiJQJ8alBFkJaUtWy98s4so1JkVf1UrD1ZPJqk0q+qXiG62kCntA38KL89ByJtOv4W UvZQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1764225328; x=1764830128; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-gg:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=QG1ab1vJmGEim6W8b8KLS1C7JN6Szi3cb3yM3drSkzs=; b=bbTxorZQ9d/Ne97zAQ7azP1PgKNgVmfwPfThEoOiwzftpKgDZlTu1ZW8kuhLalaiu9 B/FJMNGN6KnorlQipn/t5NbtoWdSJoa6zJHNyUBb+cuyy3gNNywxLRbAsUhLRYtPDAki WrSfbFtFGSRtb5KesHbappG+udE6jm6JOa9aF3/DJaGfJtaNyPwpw8VeTOG9ahwr/7jR iqNgkwiN10+oJA6yTj/BIVRtD/be7oIAyiB+GGWUhAIA50h6cESBVjSSs2Rh9DcDmg5N fHjXqzTBJYRv5re6DyNMzL6Xlu0ewyAPvuyYfls+pVPfl9iv/fhoA9o9UNWtBns3x0hP 29Ag== X-Forwarded-Encrypted: i=1; AJvYcCXSXmARhdu1701EJJIhrl4One4kYWhBO/1hgE8+lxhdKAMKgq0TsCtXvy8XNdFZFMcM+yOMutXRz4Bi26M=@vger.kernel.org X-Gm-Message-State: AOJu0Yxvz4FrKXGuJ3yzX/jjjrRUAYq/v7pckqa2uyLXUEgnJNbCRJnP KvDUeefCeA2mqzXmniWZDTYJxJkAAn9McgQg/80zgx7W3C2lmucttgUUPq8hnY/dpfc= X-Gm-Gg: ASbGncsfvsEcXxdVhDBSuDaUOxZunn3krQrBUDVMg+gAe+evgmC21yaPMpzBzdjUExd SX1mCpeKaLf7PsMxLZAkzVhLWUl4Ol+mfaygFZLKD6OEoQoHRZIefOiYZcJaD8M13WifYi2RheG 6p/t9d5yX7ud+q/YeI57ifJzq6+t1a4VI01tyB5htmSYtf4E4DiUEcZExEgtL2brwLzM7YpYHgf GXZ2VkKe3WhUoxAarEK6ZFdNTEFJEKFXD2RVuWx3Ne8zTumc/8U0ghNHQjuVrLoYTfmH+whzheg wEE7F7sSwkAU0zBCd7cNmvkm7qDliIP9I0WXyC3If81XzV/xW2nbwz4CvA+qs+4HFHimL62uGkN cpDlQlqX7zEBr0cl8/6m30emPDKPYt2z1xd8wOWqTGIZ43VenHEI0khofSv73BW45NwdR1GNaJk WJobH/8Uy4twI8kdvd X-Google-Smtp-Source: AGHT+IHEmibK0udfIuppA6wX8xMIzq+UUpCQsBWcIRyGDYq7O11kWqm0w9k6oetirALRl5l1cBS4iw== X-Received: by 2002:a05:6000:2411:b0:42b:3131:542f with SMTP id ffacd0b85a97d-42cc1cbcfd6mr24641070f8f.24.1764225328331; Wed, 26 Nov 2025 22:35:28 -0800 (PST) Received: from localhost ([196.207.164.177]) by smtp.gmail.com with UTF8SMTPSA id ffacd0b85a97d-42e1c5d6049sm1691335f8f.10.2025.11.26.22.35.26 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 26 Nov 2025 22:35:27 -0800 (PST) Date: Thu, 27 Nov 2025 09:35:23 +0300 From: Dan Carpenter To: Navaneeth K Cc: parthiban.veerasooran@microchip.com, christian.gromm@microchip.com, gregkh@linuxfoundation.org, linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org, abdun.nihaal@gmail.com Subject: Re: [PATCH v3] most: core: fix resource leak in most_register_interface error paths Message-ID: References: <20251126220856.3918-1-knavaneeth786@gmail.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20251126220856.3918-1-knavaneeth786@gmail.com> On Wed, Nov 26, 2025 at 10:08:56PM +0000, Navaneeth K wrote: > The function most_register_interface() did not correctly release resources > if it failed early (before device_register). In these cases, it returned > an error code immediately, leaking the memory allocated for the interface. > > Fix this by initializing the device early via device_initialize() and > calling put_device() on all error paths. This ensures the release > callback is triggered to free memory. > > Switch to using device_add() instead of device_register() to handle > the split initialization. > > Acked-by: Abdun Nihaal > Signed-off-by: Navaneeth K > --- Could we add this information to the commit message? "The most_register_interface() is expected to call put_device() on error which frees the resources allocated in the caller. The put_device() either calls release_mdev() or dim2_release(), depending on the caller." > Changes in v3: > - Dropped the USB cleanup patch as it was already applied upstream. > - Added Reported-by and Acked-by tags. > > drivers/most/core.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/drivers/most/core.c b/drivers/most/core.c > index da319d108ea1d..8635fd08035e9 100644 > --- a/drivers/most/core.c > +++ b/drivers/most/core.c > @@ -1283,18 +1283,23 @@ int most_register_interface(struct most_interface *iface) > struct most_channel *c; > > if (!iface || !iface->enqueue || !iface->configure || > - !iface->poison_channel || (iface->num_channels > MAX_CHANNELS)) > + !iface->poison_channel || (iface->num_channels > MAX_CHANNELS) || > + !iface->dev) > return -EINVAL; Don't add this NULL check. We never pass a NULL pointer here. It's just confusing to check for it. > > + device_initialize(iface->dev); > + > id = ida_alloc(&mdev_id, GFP_KERNEL); > if (id < 0) { > dev_err(iface->dev, "Failed to allocate device ID\n"); > + put_device(iface->dev); > return id; > } > > iface->p = kzalloc(sizeof(*iface->p), GFP_KERNEL); > if (!iface->p) { > ida_free(&mdev_id, id); > + put_device(iface->dev); > return -ENOMEM; This is an annoying thing because the function uses a mix of direct returns and an unwind ladder to clean up. It should just use an unwind ladder. So this should be a "goto err_free_ida;". That's unrelated to this patch so don't mix the two but if someone wanted to fix it, that would be great. KTODO: cleanup error handling in most_register_interface() > } > > @@ -1304,7 +1309,7 @@ int most_register_interface(struct most_interface *iface) > iface->dev->bus = &mostbus; > iface->dev->groups = interface_attr_groups; > dev_set_drvdata(iface->dev, iface); > - if (device_register(iface->dev)) { > + if (device_add(iface->dev)) { Unrelated and minor, but this should really be: ret = device_add(iface->dev); if (ret) { > dev_err(iface->dev, "Failed to register interface device\n"); > kfree(iface->p); > put_device(iface->dev); Again, unrelated to your patch, but there is still a bug in the error handling because if the if (device_register(&c->dev)) { call fails then we don't call list_del(&c->list) so it's a use after free. The way to review these is, does the unwind ladder match the release function, most_deregister_interface()? In this case the release function calls c->pipe0.comp->disconnect_channel() which is not necessary when we're cleaning up from probe() so that's fine. But the list_del(&c->list) part is necessary. Another very minor thing, but if I were writing this, I would move the put_device(&c->dev) before the goto. It's better to clean up partial iterations inside the loop, without doing gotos there. That way you can add new rungs to the top of the unwind ladder. I have a blog about this: https://staticthinking.wordpress.com/2022/04/28/free-the-last-thing-style/ regards, dan carpenter