From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-ed1-f49.google.com (mail-ed1-f49.google.com [209.85.208.49]) (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 83D70F9E8 for ; Sun, 14 Dec 2025 16:02:37 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.208.49 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1765728160; cv=none; b=c9QPVopYtolN4o92L2AlX/faOEszc9c7TRQX4vked2K5BnN+jXnRd5KMPH2Je9dQm3VOPr66qI8FwG0raVg8pW5chjQehBB3/goW8cp+VnUUjwBFTEu1nW3Sy2vz/UTzw33UvkhqMIBZCuQOd0/YHmVt2jPeYG3pIIqmtCUf+Ao= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1765728160; c=relaxed/simple; bh=WClbA3JP9PBCEQbLahbBZ/+egWdj23cqYt7+rBR+M8Q=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=lLA+moF/NmUIbnk3K9fOeBtoLPigRI9nGzxO74xlFvYUDPDT7Fr0JHiLBaKe35y7opr+6RGigWUPoXG48QSBM7APf7UPw9SGV5S+2cihTOoO+lx8uGelEIhUpa072g23m1awiNB9CPqcUadArW1QfqGxBHEnpDP8NuW+nFU2uGc= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=Mf7aZETZ; arc=none smtp.client-ip=209.85.208.49 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="Mf7aZETZ" Received: by mail-ed1-f49.google.com with SMTP id 4fb4d7f45d1cf-640c6577120so2885265a12.1 for ; Sun, 14 Dec 2025 08:02:37 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1765728156; x=1766332956; darn=vger.kernel.org; h=content-transfer-encoding:in-reply-to:content-language:from :references:cc:to:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=1HRjVx+w0FUfej9H+if55vg50Zyzey3BxyfSJanV6SE=; b=Mf7aZETZqpALHFu3VbM2xBuKrNXrWug9+mg7W4T27vSOyLlucNUD8oESoWt0F2BkSd mLNolIohsR43r8TMH5wKcaGvCcf6o44LjbSF61vf1oTzSfkk7KWkrgg7DQ7BRbztASO7 1UdxEJSs2bpcLfbWHCa0A8qiMVpGF1xK1BBukWg/Pp48bWthhkQCP0Xgbwe5H6hJMb6x WHzf2AcvdVBu2Js/YZH0Zro/+gAGxWrBJMMIdW3rCL9t+jk+uOixVfX18EdNi15yVXt7 MY/yh4PUw45Tw33LCswO9tqVbwdml17/39LzdSOrh7H7bknQqMZyUsfzhB+zzyNrcZCx hw8w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1765728156; x=1766332956; h=content-transfer-encoding:in-reply-to:content-language:from :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=1HRjVx+w0FUfej9H+if55vg50Zyzey3BxyfSJanV6SE=; b=GjU1eCgiZKjDxbnkh7Jtz4JTHIpLw7LHowbkrbqY2fWEHP+EBNX1Ee0qQiNzkKygqr UWZBCwkUSXPU4CN2NufkXv3cHx8wfxkgL4qFdOqzkR+IPCF3gbLO6pfAkAnfANnX3loa 8Th+yousUCd2FEKgd7+wrS34FbuwbZ28QgvFgclPrLrRhbtCVQ7dQGu7YIsKZTtzQmN/ gndT/3UTLCdcTP8OM/Vbx/C2CTYhduGhvBlXNuADM6pbeI0FR8NHrIzrUuKyoQAncfzq f/kP1l4s1J4wwkyaZZot5IErXVTcyyF8vIl2jXsJyh1n3kYwW9FuypTQ09Mhu0oXH4QB O4NQ== X-Forwarded-Encrypted: i=1; AJvYcCUYIiafrMfACsHm2fKX5b2RPRzbVeNdodLVgU3d00xpyHEu8t7K2w7R4yYjPaDZjPDpIRy0VjncEOm+e3Q=@vger.kernel.org X-Gm-Message-State: AOJu0YwR1IuMWtm2vXNtmQg8dhzY7f7+3DR2MVvhI5lu7gneqxP4aqNU qMvgScBpoJpr/nlcYbtH0YTq3PyrEbQN6O3numMTzgk4rdSzLUXwwNhQ X-Gm-Gg: AY/fxX4g2lZ4QL8LCNve/mbkch4bofrdoQduXdwwNG5D6lVj6+yRJYn7JSmJVeHesa4 GdHhd6iXX8LGgS3mo5SjfjsFyZ+iBANy0QRVWU+f+/DM4+rGLBsbWJUxN4zN/nsmHG2H4N36Qj2 /Kf5wtIYT88L3zZraYkdIzPtPYHxx7dQmFUXYrmnSq071BTeJ9tjhHwxxIN2WTjUWccwDtgKeUV wWfGGZ3FJcsquL7MlSy/sEVCZ5YPGeb5qqoqRwYLxkLd3UttFivj9a/5eeYTCw6xQDKEfHYuf7S wL1etpEsN3SU7qVPIMFDqlygdQ5uTqiswgr3YgKuFG29dYZCkNL6GBIvfF+kCa9GwjX2NIK7Sfe SZlUp85DtQTdKm56ahHIALDR7ujrqliw3S1M7T5wwhLofAaszktKny/CI0yzcHzs6i12AoNjdmL 3BQEny+vk55q+2GvNq0CpUX3Zx1ttiUiajrcCcHzM6g6xzgQrASjoUk6lvMHAnIqUPuem0fyNc X-Google-Smtp-Source: AGHT+IFWOX+q/68f3C3UaXjuHgRyTcHnGkkYvoTOTSRdTMz0p4912j7OAGU1Gsmdx9vlNh9tWKRW1Q== X-Received: by 2002:a17:907:60cb:b0:b73:880a:fdb7 with SMTP id a640c23a62f3a-b7d238fd2f5mr896492366b.35.1765728155474; Sun, 14 Dec 2025 08:02:35 -0800 (PST) Received: from [192.168.0.2] (dslb-002-205-018-238.002.205.pools.vodafone-ip.de. [2.205.18.238]) by smtp.gmail.com with ESMTPSA id a640c23a62f3a-b7cfa29be92sm1132110266b.10.2025.12.14.08.02.34 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sun, 14 Dec 2025 08:02:34 -0800 (PST) Message-ID: <39ba16a9-9b7d-4c26-91b5-cf775a7f8169@gmail.com> Date: Sun, 14 Dec 2025 17:02:33 +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 v2] net: dsa: Fix error handling in dsa_port_parse_of To: Ma Ke , andrew@lunn.ch, olteanv@gmail.com, davem@davemloft.net, edumazet@google.com, kuba@kernel.org, pabeni@redhat.com, horms@kernel.org, tobias@waldekranz.com Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org, akpm@linux-foundation.org, stable@vger.kernel.org References: <20251214131204.4684-1-make24@iscas.ac.cn> From: Jonas Gorski Content-Language: en-US In-Reply-To: <20251214131204.4684-1-make24@iscas.ac.cn> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Hi, On 12/14/25 14:12, Ma Ke wrote: > When of_find_net_device_by_node() successfully acquires a reference to Your subject is missing the () of dsa_port_parse_of() > a network device but the subsequent call to dsa_port_parse_cpu() > fails, dsa_port_parse_of() returns without releasing the reference > count on the network device. > > of_find_net_device_by_node() increments the reference count of the > returned structure, which should be balanced with a corresponding > put_device() when the reference is no longer needed. > > Found by code review. I agree with the reference not being properly released on failure, but I don't think this fix is complete. I was trying to figure out where the put_device() would happen in the success case (or on removal), and I failed to find it. Also if the (indirect) top caller of dsa_port_parse_of(), dsa_switch_probe(), fails at a later place the reference won't be released either. The only explicit put_device() that happens is in dsa_dev_to_net_device(), which seems to convert a device reference to a netdev reference via dev_hold(). But the only caller of that, dsa_port_parse() immediately calls dev_put() on it, essentially dropping all references, and then continuing using it. dsa_switch_shutdown() talks about dropping references taken via netdev_upper_dev_link(), but AFAICT this happens only after dsa_port_parse{,_of}() setup the conduit, so it looks like there could be a window without any reference held onto the conduit. So AFAICT the current state is: dsa_port_parse_of() keeps the device reference. dsa_port_parse() drops the device reference, and shortly has a dev_hold(), but it does not extend beyond the function. Therefore if my analysis is correct (which it may very well not be), the correct fix(es) here could be: dsa_port_parse{,_of}() should keep a reference via e.g. dev_hold() on success to the conduit. Or maybe they should unconditionally drop if *after* calling dsa_port_parse_cpu(), and dsa_port_parse_cpu() should take one when assigning dsa_port::conduit. Regardless, the end result should be that there is a reference on the conduit stored in dsa_port::conduit. dsa_switch_release_ports() should drop the references, as this seems to be called in all error paths of dsa_port_parse{,of} as well by dsa_switch_remove(). And maybe dsa_switch_shutdown() then also needs to drop the reference? Though it may need to then retake the reference on resume, and I don't know where that exactly should happen. Maybe it should also lookup the conduit(s) again to be correct. But here I'm more doing educated guesses then actually knowing what's correct. The alternative/quick "fix" would be to just drop the reference unconditionally, which would align the behaviour to that of dsa_port_parse(). Not sure if it should mirror the dev_hold() / dev_put() spiel as well. Not that I think this would be the correct behaviour though. Sorry for the lengthy review/train of thought. Best regards, Jonas > > Cc: stable@vger.kernel.org > Fixes: deff710703d8 ("net: dsa: Allow default tag protocol to be overridden from DT") > Signed-off-by: Ma Ke > --- > Changes in v2: > - simplified the patch as suggestions; > - modified the Fixes tag as suggestions. > --- > net/dsa/dsa.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c > index a20efabe778f..31b409a47491 100644 > --- a/net/dsa/dsa.c > +++ b/net/dsa/dsa.c > @@ -1247,6 +1247,7 @@ static int dsa_port_parse_of(struct dsa_port *dp, struct device_node *dn) > struct device_node *ethernet = of_parse_phandle(dn, "ethernet", 0); > const char *name = of_get_property(dn, "label", NULL); > bool link = of_property_read_bool(dn, "link"); > + int err = 0; > > dp->dn = dn; > > @@ -1260,7 +1261,11 @@ static int dsa_port_parse_of(struct dsa_port *dp, struct device_node *dn) > return -EPROBE_DEFER; > > user_protocol = of_get_property(dn, "dsa-tag-protocol", NULL); > - return dsa_port_parse_cpu(dp, conduit, user_protocol); > + err = dsa_port_parse_cpu(dp, conduit, user_protocol); > + if (err) > + put_device(conduit); > + > + return err; > } > > if (link)