NetBSD Problem Report #58165

From www@netbsd.org  Wed Apr 17 17:33:03 2024
Return-Path: <www@netbsd.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits))
	(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id C7A8B1A9238
	for <gnats-bugs@gnats.NetBSD.org>; Wed, 17 Apr 2024 17:33:03 +0000 (UTC)
Message-Id: <20240417173302.3437C1A923A@mollari.NetBSD.org>
Date: Wed, 17 Apr 2024 17:33:02 +0000 (UTC)
From: campbell+netbsd@mumble.net
Reply-To: campbell+netbsd@mumble.net
To: gnats-bugs@NetBSD.org
Subject: dead or buggy branches in vchiq_close
X-Send-Pr-Version: www-1.0

>Number:         58165
>Category:       port-arm
>Synopsis:       dead or buggy branches in vchiq_close
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    port-arm-maintainer
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Wed Apr 17 17:35:02 +0000 2024
>Originator:     Taylor R Campbell
>Release:        current, 10, 9,
>Organization:
The NetBSD Vchiqtion
>Environment:
>Description:
vchiq_fileops is only used if:

(a) the device minor exists in vchiq_open, and
(b) the global vchiq state is initialized according to vchiq_get_state in vchiq_open.

For (a), we should add `.d_cfdriver = &vchiq_cd, .d_devtounit = dev_minor_unit' to vchiq_cdevsw to avoid open/detach races.

Given (b), this branch in vchiq_close is almost certainly dead:

   1216 		VCHIQ_STATE_T *state = vchiq_get_state();
...
   1224 		if (!state) {
   1225 			ret = -EPERM;
   1226 			goto out;
   1227 		}

https://nxr.netbsd.org/xref/src/sys/external/bsd/vchiq/dist/interface/vchiq_arm/vchiq_arm.c?r=1.21#1210

If it's not dead, it's probably a memory leak, because struct fileops::fo_close is final -- it is never given a second chance to close if the first one had some transient error.

There is also another failure branch in vchiq_close that is obviously dead:

   1214 	if (1) {
...
   1332 	}
   1333 	else {
   1334 		vchiq_log_error(vchiq_arm_log_level,
   1335 			"Unknown minor device");
   1336 		ret = -ENXIO;
   1337 	}

I assume this is here for the sake of reducing merge conflicts with an upstream; if not, it should be pruned.
>How-To-Repeat:
code inspection
>Fix:
Yes, please!

1. add `.d_cfdriver = &vchiq_cd, .d_devtounit = dev_minor_unit' to vchiq_cdevsw
2. prune dead branches in vchiq_close

Home	PR Database Search