Message ID | 20230309160201.5163-1-tzimmermann@suse.de |
---|---|
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:5915:0:0:0:0:0 with SMTP id v21csp372341wrd; Thu, 9 Mar 2023 08:04:51 -0800 (PST) X-Google-Smtp-Source: AK7set//Eh9tVrLgPbvUU3fVkdyqAawj5LX3CurmmiPbZ1zOXtimO5mbwBLHDjJD2GcOz7sDscAq X-Received: by 2002:a17:90a:520c:b0:233:c301:32b3 with SMTP id v12-20020a17090a520c00b00233c30132b3mr21836480pjh.3.1678377890836; Thu, 09 Mar 2023 08:04:50 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1678377890; cv=none; d=google.com; s=arc-20160816; b=cgVOA/XPmFSqMHPONxiS5nTHrbEbHtCT6W2sVcrkrooTtYuuA4CV6mQQGMU+CAsxTx oNlS3qN+2BKRgjrnbXzDprU4aydPafgXPNJSn0AN3vqhvAe8tIhzP2g4JNlUlt30/9m/ VoJbUCDW44hfrUq7yguXZ7qoJR6RKTiCCwJg5iytRquoxvemXCpchCNkX7YjM6iZrpg6 3941ssJQlm7PR8kUWuG0OgcEnPmIShQyE17PborJGNd+LdPklF4CPKVAOzHEmtpGyGkJ PBSMRoUxdapca1D2TNriRgT7eQ13zFOKw0tkwVJ0gi1FyH1EZZV1UXCH5QMqXn+UQYxA j30A== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature:dkim-signature; bh=JSy3IOe6HAwVADCc+hI/Uqf0bdfoESCu+tTk0BOF+MI=; b=jNhprUMX8QC/FT1qUgpG0zjAiRnL41gfV85X0L94+HbKNfjzfVlHitzkfalEzTL+R4 7OrzXs+vbYAb1GAc9dk9ikBkiY4ErcrFx+XO0lkpJc8tQsteLb8gi1nhD8zuPwizq8MF ek6gzQOwyye29SnTABw1yRnyAOpMFm6/p+CIEh/mRuN75Li0mPXXP8PT9wkVTkcBQkEg lqECxHLX/9YDbJ+1SQAACoGGP3AdXRjKWTd6Y66M2wi6Lu1PfMke4Fx4WmjnLtesBOuU 6RzXoFiiM2xZhGkKHDw2WH/zb7RPWXxLbANQB4QOGL018Wq0p35nELymIlsRuXIVJ6ZZ O5pA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@suse.de header.s=susede2_rsa header.b=bz6qb5Bx; dkim=neutral (no key) header.i=@suse.de header.s=susede2_ed25519 header.b=llMqymtg; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=suse.de Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id u1-20020a17090a3fc100b002373125c085si140587pjm.141.2023.03.09.08.04.33; Thu, 09 Mar 2023 08:04:50 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@suse.de header.s=susede2_rsa header.b=bz6qb5Bx; dkim=neutral (no key) header.i=@suse.de header.s=susede2_ed25519 header.b=llMqymtg; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=suse.de Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231704AbjCIQCb (ORCPT <rfc822;carlos.wei.hk@gmail.com> + 99 others); Thu, 9 Mar 2023 11:02:31 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:47218 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230288AbjCIQCI (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Thu, 9 Mar 2023 11:02:08 -0500 Received: from smtp-out1.suse.de (smtp-out1.suse.de [195.135.220.28]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 08B35F4B43; Thu, 9 Mar 2023 08:02:05 -0800 (PST) Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by smtp-out1.suse.de (Postfix) with ESMTPS id 6BE3422143; Thu, 9 Mar 2023 16:02:04 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1678377724; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version: content-transfer-encoding:content-transfer-encoding; bh=JSy3IOe6HAwVADCc+hI/Uqf0bdfoESCu+tTk0BOF+MI=; b=bz6qb5Bx+kU6fy0DFJbxLODvavLeoLYfrzPhmOW97GSASjQRENThgOtkikza8AonRjLEQt VivNH1ZGrAtanmx/1nvdrWvKpcklJsLhopsdlO1oqE1E2Q89r2fVzrienGO/l7Vgq8iaD9 4zv0CpykPLR0A/c2eQ86nhFq9hg/8M8= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1678377724; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version: content-transfer-encoding:content-transfer-encoding; bh=JSy3IOe6HAwVADCc+hI/Uqf0bdfoESCu+tTk0BOF+MI=; b=llMqymtgLc4cOviOVHdyzcRHtQ68ua2MXLLsY8y3soHCsPeOViZQmNAQQMsy09U8KYaLuf tnkykul4AHXu9tBA== Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by imap2.suse-dmz.suse.de (Postfix) with ESMTPS id 0B40C1391B; Thu, 9 Mar 2023 16:02:04 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap2.suse-dmz.suse.de with ESMTPSA id go3iAfwCCmQHbgAAMHmgww (envelope-from <tzimmermann@suse.de>); Thu, 09 Mar 2023 16:02:04 +0000 From: Thomas Zimmermann <tzimmermann@suse.de> To: deller@gmx.de, geert+renesas@glider.be, timur@kernel.org, rdunlap@infradead.org, paulus@samba.org, benh@kernel.crashing.org, linux@armlinux.org.uk, pjones@redhat.com, adaplas@gmail.com, s.hauer@pengutronix.de, shawnguo@kernel.org, mbroemme@libmpq.org, thomas@winischhofer.net, James.Bottomley@HansenPartnership.com, sudipm.mukherjee@gmail.com, teddy.wang@siliconmotion.com, corbet@lwn.net Cc: linux-fbdev@vger.kernel.org, dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, Thomas Zimmermann <tzimmermann@suse.de> Subject: [PATCH v2 000/101] fbdev: Fix memory leak in option parsing Date: Thu, 9 Mar 2023 17:00:20 +0100 Message-Id: <20230309160201.5163-1-tzimmermann@suse.de> X-Mailer: git-send-email 2.39.2 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,SPF_HELO_NONE, SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1759906775181523228?= X-GMAIL-MSGID: =?utf-8?q?1759906775181523228?= |
Series |
fbdev: Fix memory leak in option parsing
|
|
Message
Thomas Zimmermann
March 9, 2023, 4 p.m. UTC
Introduce struct option_iter and helpers to parse command-line options with comma-separated key-value pairs. Then convert fbdev drivers to the new interface. Fixes a memory leak in the parsing of the video= option. Before commit 73ce73c30ba9 ("fbdev: Transfer video= option strings to caller; clarify ownership"), a call to fb_get_options() either returned an internal string or a duplicated string; hence ownership of the string's memory buffer was not well defined, but depended on how users specified the video= option on the kernel command line. For global settings, the caller owned the returned memory and for per-driver settings, fb_get_options() owned the memory. As calling drivers were unable to detect the case, they had no option but to leak the the memory. Commit 73ce73c30ba9 ("fbdev: Transfer video= option strings to caller; clarify ownership") changed semantics to caller-owned strings. Drivers still leaked the memory, but at least ownership was clear. This patchset fixes the memory leak and changes string ownership back to fb_get_options(). Patch 1 introduces struct option_iter and a few helpers. The interface takes an option string, such as video=, in the common form value1,key2:value2,value3 etc and returns the individual comma-separated pairs. Various modules use this pattern, so the code is located under lib/. Patches 2 to 100 go through fbdev drivers and convert them to the new interface. This often requires a number of cleanups. A driver would typically refer to the option string's video mode. Such strings are now copied to driver-allocated memory so that drivers don't refer directly to the option string's memory. The option iterator then replaces manual parsing loops based on strsep(","). All driver-allocated memory is released by removing the device or unloading the module. Patch 101 finally changes the ownership of the option string to be internal to fb_get_option(); thereby fixing the memory leak. The option iterator holds its own copy of the string and is not affected by the change. Most fbdev drivers only support to parse option strings if they are built-in. I assume that's because of the original fuzzy semantics of fb_get_options(). A later patchset could change the driver to respect video= settings in any configuration. v2: * use kstrdup()/kfree() for video strings (Geert, Timur) * fix iterator docs (Randy) * update iterator interface Thomas Zimmermann (101): lib: Add option iterator fbdev/68328fb: Remove trailing whitespaces fbdev/68328fb: Remove unused option string fbdev/acornfb: Only init fb_info once fbdev/acornfb: Parse option string with struct option_iter fbdev/amifb: Duplicate video-mode option string fbdev/amifb: Parse option string with struct option_iter fbdev/arkfb: Duplicate video-mode option string fbdev/atafb: Duplicate video-mode option string fbdev/atafb: Parse option string with struct option_iter fbdev/aty: Duplicate video-mode option string fbdev/aty: Parse option string with struct option_iter fbdev/au1100fb: Parse option string with struct option_iter fbdev/au1200fb: Parse option string with struct option_iter fbdev/cirrusfb: Duplicate video-mode option string fbdev/cirrusfb: Parse option string with struct option_iter fbdev/controlfb: Remove trailing whitespaces fbdev/controlfb: Parse option string with struct option_iter fbdev/cyber2000fb: Parse option string with struct option_iter fbdev/efifb: Parse option string with struct option_iter fbdev/fm2fb: Parse option string with struct option_iter fbdev/fsl-diu-fb: Duplicate video-mode option string fbdev/fsl-diu-fb: Parse option string with struct option_iter fbdev/gbefb: Duplicate video-mode option string fbdev/gbefb: Parse option string with struct option_iter fbdev/geode: Duplicate video-mode option string fbdev/geode: Parse option string with struct option_iter fbdev/grvga: Duplicate video-mode option string fbdev/grvga: Parse option string with struct option_iter fbdev/gxt4500: Duplicate video-mode option string fbdev/hyperv_fb: Duplicate video-mode option string fbdev/i740fb: Duplicate video-mode option string fbdev/i740fb: Parse option string with struct option_iter fbdev/i810: Duplicate video-mode option string fbdev/i810: Parse option string with struct option_iter fbdev/imsttfb: Parse option string with struct option_iter fbdev/intelfb: Duplicate video-mode option string fbdev/intelfb: Parse option string with struct option_iter fbdev/imxfb: Duplicate video-mode option string fbdev/imxfb: Parse option string with struct option_iter fbdev/kyrofb: Duplicate video-mode option string fbdev/kyrofb: Parse option string with struct option_iter fbdev/macfb: Remove trailing whitespaces fbdev/macfb: Parse option string with struct option_iter fbdev/matroxfb: Parse option string with struct option_iter fbdev/mx3fb: Duplicate video-mode option string fbdev/mx3fb: Parse option string with struct option_iter fbdev/neofb: Duplicate video-mode option string fbdev/neofb: Parse option string with struct option_iter fbdev/nvidiafb: Duplicate video-mode option string fbdev/nvidiafb: Parse option string with struct option_iter fbdev/ocfb: Duplicate video-mode option string fbdev/ocfb: Parse option string with struct option_iter fbdev/omapfb: Parse option string with struct option_iter fbdev/platinumfb: Remove trailing whitespaces fbdev/platinumfb: Parse option string with struct option_iter fbdev/pm2fb: Duplicate video-mode option string fbdev/pm2fb: Parse option string with struct option_iter fbdev/pm3fb: Duplicate video-mode option string fbdev/pm3fb: Parse option string with struct option_iter fbdev/ps3fb: Duplicate video-mode option string fbdev/ps3fb: Parse option string with struct option_iter fbdev/pvr2fb: Duplicate video-mode option string fbdev/pvr2fb: Parse option string with struct option_iter fbdev/pxafb: Parse option string with struct option_iter fbdev/rivafb: Duplicate video-mode option string fbdev/rivafb: Parse option string with struct option_iter fbdev/s3fb: Duplicate video-mode option string fbdev/s3fb: Parse option string with struct option_iter fbdev/savagefb: Duplicate video-mode option string fbdev/savagefb: Parse option string with struct option_iter fbdev/sisfb: Constify mode string fbdev/sisfb: Parse option string with struct option_iter fbdev/skeletonfb: Parse option string with struct option_iter fbdev/sm712fb: Duplicate video-mode option string fbdev/sstfb: Duplicate video-mode option string fbdev/sstfb: Parse option string with struct option_iter fbdev/stifb: Remove trailing whitespaces fbdev/stifb: Constify option string fbdev/tdfxfb: Duplicate video-mode option string fbdev/tdfxfb: Parse option string with struct option_iter fbdev/tgafb: Duplicate video-mode option string fbdev/tgafb: Parse option string with struct option_iter fbdev/tmiofb: Remove unused option string fbdev/tridentfb: Duplicate video-mode option string fbdev/tridentfb: Parse option string with struct option_iter fbdev/uvesafb: Duplicate video-mode option string fbdev/uvesafb: Parse option string with struct option_iter fbdev/valkyriefb: Remove trailing whitespaces fbdev/valkyriefb: Parse option string with struct option_iter fbdev/vermilion: Remove unused option string fbdev/vesafb: Parse option string with struct option_iter fbdev/vfb: Remove trailing whitespaces fbdev/vfb: Duplicate video-mode option string fbdev/vfb: Parse option string with struct option_iter fbdev/viafb: Parse option string with struct option_iter fbdev/vt8623fb: Duplicate video-mode option string staging/sm750fb: Release g_settings in module-exit function staging/sm750fb: Duplicate video-mode option string staging/sm750fb: Parse option string with struct option_iter fbdev: Constify option strings Documentation/core-api/kernel-api.rst | 9 ++ drivers/staging/sm750fb/sm750.c | 63 ++++---- drivers/video/fbdev/68328fb.c | 24 +-- drivers/video/fbdev/acornfb.c | 23 ++- drivers/video/fbdev/amifb.c | 23 +-- drivers/video/fbdev/arkfb.c | 10 +- drivers/video/fbdev/atafb.c | 21 +-- drivers/video/fbdev/aty/aty128fb.c | 22 ++- drivers/video/fbdev/aty/atyfb_base.c | 23 ++- drivers/video/fbdev/aty/radeon_base.c | 26 +-- drivers/video/fbdev/au1100fb.c | 13 +- drivers/video/fbdev/au1200fb.c | 15 +- drivers/video/fbdev/cirrusfb.c | 30 ++-- drivers/video/fbdev/controlfb.c | 47 +++--- drivers/video/fbdev/core/fb_cmdline.c | 13 +- drivers/video/fbdev/core/modedb.c | 8 +- drivers/video/fbdev/cyber2000fb.c | 17 +- drivers/video/fbdev/efifb.c | 44 ++--- drivers/video/fbdev/ep93xx-fb.c | 2 +- drivers/video/fbdev/fm2fb.c | 14 +- drivers/video/fbdev/fsl-diu-fb.c | 24 +-- drivers/video/fbdev/gbefb.c | 23 +-- drivers/video/fbdev/geode/gx1fb_core.c | 16 +- drivers/video/fbdev/geode/gxfb_core.c | 23 +-- drivers/video/fbdev/geode/lxfb_core.c | 25 +-- drivers/video/fbdev/grvga.c | 18 ++- drivers/video/fbdev/gxt4500.c | 13 +- drivers/video/fbdev/hyperv_fb.c | 18 ++- drivers/video/fbdev/i740fb.c | 26 +-- drivers/video/fbdev/i810/i810_main.c | 26 ++- drivers/video/fbdev/imsttfb.c | 16 +- drivers/video/fbdev/imxfb.c | 21 +-- drivers/video/fbdev/intelfb/intelfbdrv.c | 23 ++- drivers/video/fbdev/kyro/fbdev.c | 21 ++- drivers/video/fbdev/macfb.c | 26 +-- drivers/video/fbdev/matrox/matroxfb_base.c | 19 +-- drivers/video/fbdev/mx3fb.c | 23 ++- drivers/video/fbdev/neofb.c | 26 +-- drivers/video/fbdev/nvidia/nvidia.c | 26 ++- drivers/video/fbdev/ocfb.c | 21 ++- drivers/video/fbdev/omap/omapfb_main.c | 15 +- drivers/video/fbdev/platinumfb.c | 44 ++--- drivers/video/fbdev/pm2fb.c | 25 +-- drivers/video/fbdev/pm3fb.c | 27 ++-- drivers/video/fbdev/ps3fb.c | 28 ++-- drivers/video/fbdev/pvr2fb.c | 32 ++-- drivers/video/fbdev/pxafb.c | 18 ++- drivers/video/fbdev/riva/fbdev.c | 26 ++- drivers/video/fbdev/s3fb.c | 27 ++-- drivers/video/fbdev/savage/savagefb_driver.c | 20 ++- drivers/video/fbdev/sis/sis_main.c | 24 +-- drivers/video/fbdev/skeletonfb.c | 17 +- drivers/video/fbdev/sm712fb.c | 12 +- drivers/video/fbdev/sstfb.c | 25 +-- drivers/video/fbdev/stifb.c | 162 +++++++++---------- drivers/video/fbdev/tdfxfb.c | 21 ++- drivers/video/fbdev/tgafb.c | 30 ++-- drivers/video/fbdev/tmiofb.c | 24 +-- drivers/video/fbdev/tridentfb.c | 27 ++-- drivers/video/fbdev/uvesafb.c | 21 ++- drivers/video/fbdev/valkyriefb.c | 30 ++-- drivers/video/fbdev/vermilion/vermilion.c | 7 +- drivers/video/fbdev/vesafb.c | 16 +- drivers/video/fbdev/vfb.c | 35 ++-- drivers/video/fbdev/via/viafbdev.c | 15 +- drivers/video/fbdev/vt8623fb.c | 11 +- include/linux/cmdline.h | 36 +++++ include/linux/fb.h | 2 +- lib/Makefile | 2 +- lib/cmdline_iter.c | 109 +++++++++++++ 70 files changed, 1087 insertions(+), 682 deletions(-) create mode 100644 include/linux/cmdline.h create mode 100644 lib/cmdline_iter.c
Comments
Hi Thomas, On Thu, Mar 9, 2023 at 5:02 PM Thomas Zimmermann <tzimmermann@suse.de> wrote: > Introduce struct option_iter and helpers to parse command-line > options with comma-separated key-value pairs. Then convert fbdev > drivers to the new interface. Fixes a memory leak in the parsing of > the video= option. > > Before commit 73ce73c30ba9 ("fbdev: Transfer video= option strings to > caller; clarify ownership"), a call to fb_get_options() either > returned an internal string or a duplicated string; hence ownership of > the string's memory buffer was not well defined, but depended on how > users specified the video= option on the kernel command line. For > global settings, the caller owned the returned memory and for per-driver > settings, fb_get_options() owned the memory. As calling drivers were > unable to detect the case, they had no option but to leak the the memory. > > Commit 73ce73c30ba9 ("fbdev: Transfer video= option strings to caller; > clarify ownership") changed semantics to caller-owned strings. Drivers > still leaked the memory, but at least ownership was clear. While I can find the actual patch[1], I cannot find this commit? Where was it applied? [1] https://lore.kernel.org/all/20230209135509.7786-3-tzimmermann@suse.de > This patchset fixes the memory leak and changes string ownership back > to fb_get_options(). Patch 1 introduces struct option_iter and a few > helpers. The interface takes an option string, such as video=, in the > common form value1,key2:value2,value3 etc and returns the individual > comma-separated pairs. Various modules use this pattern, so the code > is located under lib/. > > Patches 2 to 100 go through fbdev drivers and convert them to the new > interface. This often requires a number of cleanups. A driver would > typically refer to the option string's video mode. Such strings are now > copied to driver-allocated memory so that drivers don't refer directly > to the option string's memory. The option iterator then replaces manual > parsing loops based on strsep(","). All driver-allocated memory is > released by removing the device or unloading the module. > > Patch 101 finally changes the ownership of the option string to be > internal to fb_get_option(); thereby fixing the memory leak. The option > iterator holds its own copy of the string and is not affected by the > change. > > Most fbdev drivers only support to parse option strings if they are > built-in. I assume that's because of the original fuzzy semantics of > fb_get_options(). A later patchset could change the driver to respect > video= settings in any configuration. > > v2: > * use kstrdup()/kfree() for video strings (Geert, Timur) > * fix iterator docs (Randy) > * update iterator interface Thanks for the update, this looks much better! Gr{oetje,eeting}s, Geert
Hi Geert Am 10.03.23 um 09:24 schrieb Geert Uytterhoeven: > Hi Thomas, > > On Thu, Mar 9, 2023 at 5:02 PM Thomas Zimmermann <tzimmermann@suse.de> wrote: >> Introduce struct option_iter and helpers to parse command-line >> options with comma-separated key-value pairs. Then convert fbdev >> drivers to the new interface. Fixes a memory leak in the parsing of >> the video= option. >> >> Before commit 73ce73c30ba9 ("fbdev: Transfer video= option strings to >> caller; clarify ownership"), a call to fb_get_options() either >> returned an internal string or a duplicated string; hence ownership of >> the string's memory buffer was not well defined, but depended on how >> users specified the video= option on the kernel command line. For >> global settings, the caller owned the returned memory and for per-driver >> settings, fb_get_options() owned the memory. As calling drivers were >> unable to detect the case, they had no option but to leak the the memory. >> >> Commit 73ce73c30ba9 ("fbdev: Transfer video= option strings to caller; >> clarify ownership") changed semantics to caller-owned strings. Drivers >> still leaked the memory, but at least ownership was clear. > > While I can find the actual patch[1], I cannot find this commit? > Where was it applied? It's currently in drm-misc-next. https://cgit.freedesktop.org/drm/drm-misc/commit/?id=73ce73c30ba9ae4d90fdfad7ebe9104001d5d851 Best regards Thomas > > [1] https://lore.kernel.org/all/20230209135509.7786-3-tzimmermann@suse.de > >> This patchset fixes the memory leak and changes string ownership back >> to fb_get_options(). Patch 1 introduces struct option_iter and a few >> helpers. The interface takes an option string, such as video=, in the >> common form value1,key2:value2,value3 etc and returns the individual >> comma-separated pairs. Various modules use this pattern, so the code >> is located under lib/. >> >> Patches 2 to 100 go through fbdev drivers and convert them to the new >> interface. This often requires a number of cleanups. A driver would >> typically refer to the option string's video mode. Such strings are now >> copied to driver-allocated memory so that drivers don't refer directly >> to the option string's memory. The option iterator then replaces manual >> parsing loops based on strsep(","). All driver-allocated memory is >> released by removing the device or unloading the module. >> >> Patch 101 finally changes the ownership of the option string to be >> internal to fb_get_option(); thereby fixing the memory leak. The option >> iterator holds its own copy of the string and is not affected by the >> change. >> >> Most fbdev drivers only support to parse option strings if they are >> built-in. I assume that's because of the original fuzzy semantics of >> fb_get_options(). A later patchset could change the driver to respect >> video= settings in any configuration. >> >> v2: >> * use kstrdup()/kfree() for video strings (Geert, Timur) >> * fix iterator docs (Randy) >> * update iterator interface > > Thanks for the update, this looks much better! > > Gr{oetje,eeting}s, > > Geert > -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Ivo Totev
Geert, Helge? Do you have further comments? There's not really much for a v3 yet. Best regards Thomas Am 09.03.23 um 17:00 schrieb Thomas Zimmermann: > Introduce struct option_iter and helpers to parse command-line > options with comma-separated key-value pairs. Then convert fbdev > drivers to the new interface. Fixes a memory leak in the parsing of > the video= option. > > Before commit 73ce73c30ba9 ("fbdev: Transfer video= option strings to > caller; clarify ownership"), a call to fb_get_options() either > returned an internal string or a duplicated string; hence ownership of > the string's memory buffer was not well defined, but depended on how > users specified the video= option on the kernel command line. For > global settings, the caller owned the returned memory and for per-driver > settings, fb_get_options() owned the memory. As calling drivers were > unable to detect the case, they had no option but to leak the the memory. > > Commit 73ce73c30ba9 ("fbdev: Transfer video= option strings to caller; > clarify ownership") changed semantics to caller-owned strings. Drivers > still leaked the memory, but at least ownership was clear. > > This patchset fixes the memory leak and changes string ownership back > to fb_get_options(). Patch 1 introduces struct option_iter and a few > helpers. The interface takes an option string, such as video=, in the > common form value1,key2:value2,value3 etc and returns the individual > comma-separated pairs. Various modules use this pattern, so the code > is located under lib/. > > Patches 2 to 100 go through fbdev drivers and convert them to the new > interface. This often requires a number of cleanups. A driver would > typically refer to the option string's video mode. Such strings are now > copied to driver-allocated memory so that drivers don't refer directly > to the option string's memory. The option iterator then replaces manual > parsing loops based on strsep(","). All driver-allocated memory is > released by removing the device or unloading the module. > > Patch 101 finally changes the ownership of the option string to be > internal to fb_get_option(); thereby fixing the memory leak. The option > iterator holds its own copy of the string and is not affected by the > change. > > Most fbdev drivers only support to parse option strings if they are > built-in. I assume that's because of the original fuzzy semantics of > fb_get_options(). A later patchset could change the driver to respect > video= settings in any configuration. > > v2: > * use kstrdup()/kfree() for video strings (Geert, Timur) > * fix iterator docs (Randy) > * update iterator interface > > Thomas Zimmermann (101): > lib: Add option iterator > fbdev/68328fb: Remove trailing whitespaces > fbdev/68328fb: Remove unused option string > fbdev/acornfb: Only init fb_info once > fbdev/acornfb: Parse option string with struct option_iter > fbdev/amifb: Duplicate video-mode option string > fbdev/amifb: Parse option string with struct option_iter > fbdev/arkfb: Duplicate video-mode option string > fbdev/atafb: Duplicate video-mode option string > fbdev/atafb: Parse option string with struct option_iter > fbdev/aty: Duplicate video-mode option string > fbdev/aty: Parse option string with struct option_iter > fbdev/au1100fb: Parse option string with struct option_iter > fbdev/au1200fb: Parse option string with struct option_iter > fbdev/cirrusfb: Duplicate video-mode option string > fbdev/cirrusfb: Parse option string with struct option_iter > fbdev/controlfb: Remove trailing whitespaces > fbdev/controlfb: Parse option string with struct option_iter > fbdev/cyber2000fb: Parse option string with struct option_iter > fbdev/efifb: Parse option string with struct option_iter > fbdev/fm2fb: Parse option string with struct option_iter > fbdev/fsl-diu-fb: Duplicate video-mode option string > fbdev/fsl-diu-fb: Parse option string with struct option_iter > fbdev/gbefb: Duplicate video-mode option string > fbdev/gbefb: Parse option string with struct option_iter > fbdev/geode: Duplicate video-mode option string > fbdev/geode: Parse option string with struct option_iter > fbdev/grvga: Duplicate video-mode option string > fbdev/grvga: Parse option string with struct option_iter > fbdev/gxt4500: Duplicate video-mode option string > fbdev/hyperv_fb: Duplicate video-mode option string > fbdev/i740fb: Duplicate video-mode option string > fbdev/i740fb: Parse option string with struct option_iter > fbdev/i810: Duplicate video-mode option string > fbdev/i810: Parse option string with struct option_iter > fbdev/imsttfb: Parse option string with struct option_iter > fbdev/intelfb: Duplicate video-mode option string > fbdev/intelfb: Parse option string with struct option_iter > fbdev/imxfb: Duplicate video-mode option string > fbdev/imxfb: Parse option string with struct option_iter > fbdev/kyrofb: Duplicate video-mode option string > fbdev/kyrofb: Parse option string with struct option_iter > fbdev/macfb: Remove trailing whitespaces > fbdev/macfb: Parse option string with struct option_iter > fbdev/matroxfb: Parse option string with struct option_iter > fbdev/mx3fb: Duplicate video-mode option string > fbdev/mx3fb: Parse option string with struct option_iter > fbdev/neofb: Duplicate video-mode option string > fbdev/neofb: Parse option string with struct option_iter > fbdev/nvidiafb: Duplicate video-mode option string > fbdev/nvidiafb: Parse option string with struct option_iter > fbdev/ocfb: Duplicate video-mode option string > fbdev/ocfb: Parse option string with struct option_iter > fbdev/omapfb: Parse option string with struct option_iter > fbdev/platinumfb: Remove trailing whitespaces > fbdev/platinumfb: Parse option string with struct option_iter > fbdev/pm2fb: Duplicate video-mode option string > fbdev/pm2fb: Parse option string with struct option_iter > fbdev/pm3fb: Duplicate video-mode option string > fbdev/pm3fb: Parse option string with struct option_iter > fbdev/ps3fb: Duplicate video-mode option string > fbdev/ps3fb: Parse option string with struct option_iter > fbdev/pvr2fb: Duplicate video-mode option string > fbdev/pvr2fb: Parse option string with struct option_iter > fbdev/pxafb: Parse option string with struct option_iter > fbdev/rivafb: Duplicate video-mode option string > fbdev/rivafb: Parse option string with struct option_iter > fbdev/s3fb: Duplicate video-mode option string > fbdev/s3fb: Parse option string with struct option_iter > fbdev/savagefb: Duplicate video-mode option string > fbdev/savagefb: Parse option string with struct option_iter > fbdev/sisfb: Constify mode string > fbdev/sisfb: Parse option string with struct option_iter > fbdev/skeletonfb: Parse option string with struct option_iter > fbdev/sm712fb: Duplicate video-mode option string > fbdev/sstfb: Duplicate video-mode option string > fbdev/sstfb: Parse option string with struct option_iter > fbdev/stifb: Remove trailing whitespaces > fbdev/stifb: Constify option string > fbdev/tdfxfb: Duplicate video-mode option string > fbdev/tdfxfb: Parse option string with struct option_iter > fbdev/tgafb: Duplicate video-mode option string > fbdev/tgafb: Parse option string with struct option_iter > fbdev/tmiofb: Remove unused option string > fbdev/tridentfb: Duplicate video-mode option string > fbdev/tridentfb: Parse option string with struct option_iter > fbdev/uvesafb: Duplicate video-mode option string > fbdev/uvesafb: Parse option string with struct option_iter > fbdev/valkyriefb: Remove trailing whitespaces > fbdev/valkyriefb: Parse option string with struct option_iter > fbdev/vermilion: Remove unused option string > fbdev/vesafb: Parse option string with struct option_iter > fbdev/vfb: Remove trailing whitespaces > fbdev/vfb: Duplicate video-mode option string > fbdev/vfb: Parse option string with struct option_iter > fbdev/viafb: Parse option string with struct option_iter > fbdev/vt8623fb: Duplicate video-mode option string > staging/sm750fb: Release g_settings in module-exit function > staging/sm750fb: Duplicate video-mode option string > staging/sm750fb: Parse option string with struct option_iter > fbdev: Constify option strings > > Documentation/core-api/kernel-api.rst | 9 ++ > drivers/staging/sm750fb/sm750.c | 63 ++++---- > drivers/video/fbdev/68328fb.c | 24 +-- > drivers/video/fbdev/acornfb.c | 23 ++- > drivers/video/fbdev/amifb.c | 23 +-- > drivers/video/fbdev/arkfb.c | 10 +- > drivers/video/fbdev/atafb.c | 21 +-- > drivers/video/fbdev/aty/aty128fb.c | 22 ++- > drivers/video/fbdev/aty/atyfb_base.c | 23 ++- > drivers/video/fbdev/aty/radeon_base.c | 26 +-- > drivers/video/fbdev/au1100fb.c | 13 +- > drivers/video/fbdev/au1200fb.c | 15 +- > drivers/video/fbdev/cirrusfb.c | 30 ++-- > drivers/video/fbdev/controlfb.c | 47 +++--- > drivers/video/fbdev/core/fb_cmdline.c | 13 +- > drivers/video/fbdev/core/modedb.c | 8 +- > drivers/video/fbdev/cyber2000fb.c | 17 +- > drivers/video/fbdev/efifb.c | 44 ++--- > drivers/video/fbdev/ep93xx-fb.c | 2 +- > drivers/video/fbdev/fm2fb.c | 14 +- > drivers/video/fbdev/fsl-diu-fb.c | 24 +-- > drivers/video/fbdev/gbefb.c | 23 +-- > drivers/video/fbdev/geode/gx1fb_core.c | 16 +- > drivers/video/fbdev/geode/gxfb_core.c | 23 +-- > drivers/video/fbdev/geode/lxfb_core.c | 25 +-- > drivers/video/fbdev/grvga.c | 18 ++- > drivers/video/fbdev/gxt4500.c | 13 +- > drivers/video/fbdev/hyperv_fb.c | 18 ++- > drivers/video/fbdev/i740fb.c | 26 +-- > drivers/video/fbdev/i810/i810_main.c | 26 ++- > drivers/video/fbdev/imsttfb.c | 16 +- > drivers/video/fbdev/imxfb.c | 21 +-- > drivers/video/fbdev/intelfb/intelfbdrv.c | 23 ++- > drivers/video/fbdev/kyro/fbdev.c | 21 ++- > drivers/video/fbdev/macfb.c | 26 +-- > drivers/video/fbdev/matrox/matroxfb_base.c | 19 +-- > drivers/video/fbdev/mx3fb.c | 23 ++- > drivers/video/fbdev/neofb.c | 26 +-- > drivers/video/fbdev/nvidia/nvidia.c | 26 ++- > drivers/video/fbdev/ocfb.c | 21 ++- > drivers/video/fbdev/omap/omapfb_main.c | 15 +- > drivers/video/fbdev/platinumfb.c | 44 ++--- > drivers/video/fbdev/pm2fb.c | 25 +-- > drivers/video/fbdev/pm3fb.c | 27 ++-- > drivers/video/fbdev/ps3fb.c | 28 ++-- > drivers/video/fbdev/pvr2fb.c | 32 ++-- > drivers/video/fbdev/pxafb.c | 18 ++- > drivers/video/fbdev/riva/fbdev.c | 26 ++- > drivers/video/fbdev/s3fb.c | 27 ++-- > drivers/video/fbdev/savage/savagefb_driver.c | 20 ++- > drivers/video/fbdev/sis/sis_main.c | 24 +-- > drivers/video/fbdev/skeletonfb.c | 17 +- > drivers/video/fbdev/sm712fb.c | 12 +- > drivers/video/fbdev/sstfb.c | 25 +-- > drivers/video/fbdev/stifb.c | 162 +++++++++---------- > drivers/video/fbdev/tdfxfb.c | 21 ++- > drivers/video/fbdev/tgafb.c | 30 ++-- > drivers/video/fbdev/tmiofb.c | 24 +-- > drivers/video/fbdev/tridentfb.c | 27 ++-- > drivers/video/fbdev/uvesafb.c | 21 ++- > drivers/video/fbdev/valkyriefb.c | 30 ++-- > drivers/video/fbdev/vermilion/vermilion.c | 7 +- > drivers/video/fbdev/vesafb.c | 16 +- > drivers/video/fbdev/vfb.c | 35 ++-- > drivers/video/fbdev/via/viafbdev.c | 15 +- > drivers/video/fbdev/vt8623fb.c | 11 +- > include/linux/cmdline.h | 36 +++++ > include/linux/fb.h | 2 +- > lib/Makefile | 2 +- > lib/cmdline_iter.c | 109 +++++++++++++ > 70 files changed, 1087 insertions(+), 682 deletions(-) > create mode 100644 include/linux/cmdline.h > create mode 100644 lib/cmdline_iter.c > -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Ivo Totev
Hi Thomas, On 3/20/23 11:07, Thomas Zimmermann wrote: > Geert, Helge? Do you have further comments? There's not really much for a v3 yet. I understand the motivation and I see you invested a lot of work on it, which is really appreciated. But I have mixed feelings about that patch itself. Nevertheless, it mixes multiple things. Regarding the possible memory leak (for the parameter) you added in various places a kfree(), but this doesn't make it easier for the driver author to know when to free and when not. I wonder if it would be possible to store the kstrdup() value in the "struct module" (or somewhere module related) instead add kfree it at a central place when the module is unloaded (instead inside the driver itself). That way no driver needs to be touched and there are less changes necessary. And it would change it back to globally-owned strings. With such a change I think the new command parsing functions wouldn't be needed either, and ideally, where possible, a conversion to module_param() in various places would be better. I'm sure I haven't overlooked everything yet, but I think if we can reduce the necessary changes by fixing things globally it makes more sense. Helge > > Best regards > Thomas > > Am 09.03.23 um 17:00 schrieb Thomas Zimmermann: >> Introduce struct option_iter and helpers to parse command-line >> options with comma-separated key-value pairs. Then convert fbdev >> drivers to the new interface. Fixes a memory leak in the parsing of >> the video= option. >> >> Before commit 73ce73c30ba9 ("fbdev: Transfer video= option strings to >> caller; clarify ownership"), a call to fb_get_options() either >> returned an internal string or a duplicated string; hence ownership of >> the string's memory buffer was not well defined, but depended on how >> users specified the video= option on the kernel command line. For >> global settings, the caller owned the returned memory and for per-driver >> settings, fb_get_options() owned the memory. As calling drivers were >> unable to detect the case, they had no option but to leak the the memory. >> >> Commit 73ce73c30ba9 ("fbdev: Transfer video= option strings to caller; >> clarify ownership") changed semantics to caller-owned strings. Drivers >> still leaked the memory, but at least ownership was clear. >> >> This patchset fixes the memory leak and changes string ownership back >> to fb_get_options(). Patch 1 introduces struct option_iter and a few >> helpers. The interface takes an option string, such as video=, in the >> common form value1,key2:value2,value3 etc and returns the individual >> comma-separated pairs. Various modules use this pattern, so the code >> is located under lib/. >> >> Patches 2 to 100 go through fbdev drivers and convert them to the new >> interface. This often requires a number of cleanups. A driver would >> typically refer to the option string's video mode. Such strings are now >> copied to driver-allocated memory so that drivers don't refer directly >> to the option string's memory. The option iterator then replaces manual >> parsing loops based on strsep(","). All driver-allocated memory is >> released by removing the device or unloading the module. >> >> Patch 101 finally changes the ownership of the option string to be >> internal to fb_get_option(); thereby fixing the memory leak. The option >> iterator holds its own copy of the string and is not affected by the >> change. >> >> Most fbdev drivers only support to parse option strings if they are >> built-in. I assume that's because of the original fuzzy semantics of >> fb_get_options(). A later patchset could change the driver to respect >> video= settings in any configuration. >> >> v2: >> * use kstrdup()/kfree() for video strings (Geert, Timur) >> * fix iterator docs (Randy) >> * update iterator interface >> >> Thomas Zimmermann (101): >> lib: Add option iterator >> fbdev/68328fb: Remove trailing whitespaces >> fbdev/68328fb: Remove unused option string >> fbdev/acornfb: Only init fb_info once >> fbdev/acornfb: Parse option string with struct option_iter >> fbdev/amifb: Duplicate video-mode option string >> fbdev/amifb: Parse option string with struct option_iter >> fbdev/arkfb: Duplicate video-mode option string >> fbdev/atafb: Duplicate video-mode option string >> fbdev/atafb: Parse option string with struct option_iter >> fbdev/aty: Duplicate video-mode option string >> fbdev/aty: Parse option string with struct option_iter >> fbdev/au1100fb: Parse option string with struct option_iter >> fbdev/au1200fb: Parse option string with struct option_iter >> fbdev/cirrusfb: Duplicate video-mode option string >> fbdev/cirrusfb: Parse option string with struct option_iter >> fbdev/controlfb: Remove trailing whitespaces >> fbdev/controlfb: Parse option string with struct option_iter >> fbdev/cyber2000fb: Parse option string with struct option_iter >> fbdev/efifb: Parse option string with struct option_iter >> fbdev/fm2fb: Parse option string with struct option_iter >> fbdev/fsl-diu-fb: Duplicate video-mode option string >> fbdev/fsl-diu-fb: Parse option string with struct option_iter >> fbdev/gbefb: Duplicate video-mode option string >> fbdev/gbefb: Parse option string with struct option_iter >> fbdev/geode: Duplicate video-mode option string >> fbdev/geode: Parse option string with struct option_iter >> fbdev/grvga: Duplicate video-mode option string >> fbdev/grvga: Parse option string with struct option_iter >> fbdev/gxt4500: Duplicate video-mode option string >> fbdev/hyperv_fb: Duplicate video-mode option string >> fbdev/i740fb: Duplicate video-mode option string >> fbdev/i740fb: Parse option string with struct option_iter >> fbdev/i810: Duplicate video-mode option string >> fbdev/i810: Parse option string with struct option_iter >> fbdev/imsttfb: Parse option string with struct option_iter >> fbdev/intelfb: Duplicate video-mode option string >> fbdev/intelfb: Parse option string with struct option_iter >> fbdev/imxfb: Duplicate video-mode option string >> fbdev/imxfb: Parse option string with struct option_iter >> fbdev/kyrofb: Duplicate video-mode option string >> fbdev/kyrofb: Parse option string with struct option_iter >> fbdev/macfb: Remove trailing whitespaces >> fbdev/macfb: Parse option string with struct option_iter >> fbdev/matroxfb: Parse option string with struct option_iter >> fbdev/mx3fb: Duplicate video-mode option string >> fbdev/mx3fb: Parse option string with struct option_iter >> fbdev/neofb: Duplicate video-mode option string >> fbdev/neofb: Parse option string with struct option_iter >> fbdev/nvidiafb: Duplicate video-mode option string >> fbdev/nvidiafb: Parse option string with struct option_iter >> fbdev/ocfb: Duplicate video-mode option string >> fbdev/ocfb: Parse option string with struct option_iter >> fbdev/omapfb: Parse option string with struct option_iter >> fbdev/platinumfb: Remove trailing whitespaces >> fbdev/platinumfb: Parse option string with struct option_iter >> fbdev/pm2fb: Duplicate video-mode option string >> fbdev/pm2fb: Parse option string with struct option_iter >> fbdev/pm3fb: Duplicate video-mode option string >> fbdev/pm3fb: Parse option string with struct option_iter >> fbdev/ps3fb: Duplicate video-mode option string >> fbdev/ps3fb: Parse option string with struct option_iter >> fbdev/pvr2fb: Duplicate video-mode option string >> fbdev/pvr2fb: Parse option string with struct option_iter >> fbdev/pxafb: Parse option string with struct option_iter >> fbdev/rivafb: Duplicate video-mode option string >> fbdev/rivafb: Parse option string with struct option_iter >> fbdev/s3fb: Duplicate video-mode option string >> fbdev/s3fb: Parse option string with struct option_iter >> fbdev/savagefb: Duplicate video-mode option string >> fbdev/savagefb: Parse option string with struct option_iter >> fbdev/sisfb: Constify mode string >> fbdev/sisfb: Parse option string with struct option_iter >> fbdev/skeletonfb: Parse option string with struct option_iter >> fbdev/sm712fb: Duplicate video-mode option string >> fbdev/sstfb: Duplicate video-mode option string >> fbdev/sstfb: Parse option string with struct option_iter >> fbdev/stifb: Remove trailing whitespaces >> fbdev/stifb: Constify option string >> fbdev/tdfxfb: Duplicate video-mode option string >> fbdev/tdfxfb: Parse option string with struct option_iter >> fbdev/tgafb: Duplicate video-mode option string >> fbdev/tgafb: Parse option string with struct option_iter >> fbdev/tmiofb: Remove unused option string >> fbdev/tridentfb: Duplicate video-mode option string >> fbdev/tridentfb: Parse option string with struct option_iter >> fbdev/uvesafb: Duplicate video-mode option string >> fbdev/uvesafb: Parse option string with struct option_iter >> fbdev/valkyriefb: Remove trailing whitespaces >> fbdev/valkyriefb: Parse option string with struct option_iter >> fbdev/vermilion: Remove unused option string >> fbdev/vesafb: Parse option string with struct option_iter >> fbdev/vfb: Remove trailing whitespaces >> fbdev/vfb: Duplicate video-mode option string >> fbdev/vfb: Parse option string with struct option_iter >> fbdev/viafb: Parse option string with struct option_iter >> fbdev/vt8623fb: Duplicate video-mode option string >> staging/sm750fb: Release g_settings in module-exit function >> staging/sm750fb: Duplicate video-mode option string >> staging/sm750fb: Parse option string with struct option_iter >> fbdev: Constify option strings >> >> Documentation/core-api/kernel-api.rst | 9 ++ >> drivers/staging/sm750fb/sm750.c | 63 ++++---- >> drivers/video/fbdev/68328fb.c | 24 +-- >> drivers/video/fbdev/acornfb.c | 23 ++- >> drivers/video/fbdev/amifb.c | 23 +-- >> drivers/video/fbdev/arkfb.c | 10 +- >> drivers/video/fbdev/atafb.c | 21 +-- >> drivers/video/fbdev/aty/aty128fb.c | 22 ++- >> drivers/video/fbdev/aty/atyfb_base.c | 23 ++- >> drivers/video/fbdev/aty/radeon_base.c | 26 +-- >> drivers/video/fbdev/au1100fb.c | 13 +- >> drivers/video/fbdev/au1200fb.c | 15 +- >> drivers/video/fbdev/cirrusfb.c | 30 ++-- >> drivers/video/fbdev/controlfb.c | 47 +++--- >> drivers/video/fbdev/core/fb_cmdline.c | 13 +- >> drivers/video/fbdev/core/modedb.c | 8 +- >> drivers/video/fbdev/cyber2000fb.c | 17 +- >> drivers/video/fbdev/efifb.c | 44 ++--- >> drivers/video/fbdev/ep93xx-fb.c | 2 +- >> drivers/video/fbdev/fm2fb.c | 14 +- >> drivers/video/fbdev/fsl-diu-fb.c | 24 +-- >> drivers/video/fbdev/gbefb.c | 23 +-- >> drivers/video/fbdev/geode/gx1fb_core.c | 16 +- >> drivers/video/fbdev/geode/gxfb_core.c | 23 +-- >> drivers/video/fbdev/geode/lxfb_core.c | 25 +-- >> drivers/video/fbdev/grvga.c | 18 ++- >> drivers/video/fbdev/gxt4500.c | 13 +- >> drivers/video/fbdev/hyperv_fb.c | 18 ++- >> drivers/video/fbdev/i740fb.c | 26 +-- >> drivers/video/fbdev/i810/i810_main.c | 26 ++- >> drivers/video/fbdev/imsttfb.c | 16 +- >> drivers/video/fbdev/imxfb.c | 21 +-- >> drivers/video/fbdev/intelfb/intelfbdrv.c | 23 ++- >> drivers/video/fbdev/kyro/fbdev.c | 21 ++- >> drivers/video/fbdev/macfb.c | 26 +-- >> drivers/video/fbdev/matrox/matroxfb_base.c | 19 +-- >> drivers/video/fbdev/mx3fb.c | 23 ++- >> drivers/video/fbdev/neofb.c | 26 +-- >> drivers/video/fbdev/nvidia/nvidia.c | 26 ++- >> drivers/video/fbdev/ocfb.c | 21 ++- >> drivers/video/fbdev/omap/omapfb_main.c | 15 +- >> drivers/video/fbdev/platinumfb.c | 44 ++--- >> drivers/video/fbdev/pm2fb.c | 25 +-- >> drivers/video/fbdev/pm3fb.c | 27 ++-- >> drivers/video/fbdev/ps3fb.c | 28 ++-- >> drivers/video/fbdev/pvr2fb.c | 32 ++-- >> drivers/video/fbdev/pxafb.c | 18 ++- >> drivers/video/fbdev/riva/fbdev.c | 26 ++- >> drivers/video/fbdev/s3fb.c | 27 ++-- >> drivers/video/fbdev/savage/savagefb_driver.c | 20 ++- >> drivers/video/fbdev/sis/sis_main.c | 24 +-- >> drivers/video/fbdev/skeletonfb.c | 17 +- >> drivers/video/fbdev/sm712fb.c | 12 +- >> drivers/video/fbdev/sstfb.c | 25 +-- >> drivers/video/fbdev/stifb.c | 162 +++++++++---------- >> drivers/video/fbdev/tdfxfb.c | 21 ++- >> drivers/video/fbdev/tgafb.c | 30 ++-- >> drivers/video/fbdev/tmiofb.c | 24 +-- >> drivers/video/fbdev/tridentfb.c | 27 ++-- >> drivers/video/fbdev/uvesafb.c | 21 ++- >> drivers/video/fbdev/valkyriefb.c | 30 ++-- >> drivers/video/fbdev/vermilion/vermilion.c | 7 +- >> drivers/video/fbdev/vesafb.c | 16 +- >> drivers/video/fbdev/vfb.c | 35 ++-- >> drivers/video/fbdev/via/viafbdev.c | 15 +- >> drivers/video/fbdev/vt8623fb.c | 11 +- >> include/linux/cmdline.h | 36 +++++ >> include/linux/fb.h | 2 +- >> lib/Makefile | 2 +- >> lib/cmdline_iter.c | 109 +++++++++++++ >> 70 files changed, 1087 insertions(+), 682 deletions(-) >> create mode 100644 include/linux/cmdline.h >> create mode 100644 lib/cmdline_iter.c >> >
Hi Helge, thanks for the feedback. Am 20.03.23 um 20:25 schrieb Helge Deller: > Hi Thomas, > > On 3/20/23 11:07, Thomas Zimmermann wrote: >> Geert, Helge? Do you have further comments? There's not really much >> for a v3 yet. > > I understand the motivation and I see you invested a lot of work on it, > which is really appreciated. > But I have mixed feelings about that patch itself. Can I have an ack on the patches that fix trailing whitespaces? Those should be uncontroversion. > > Nevertheless, it mixes multiple things. > Regarding the possible memory leak (for the parameter) you added in various > places a kfree(), but this doesn't make it easier for the driver author > to know > when to free and when not. > I wonder if it would be possible to store the kstrdup() value in the > "struct module" (or somewhere module related) instead add kfree it at a > central place when the > module is unloaded (instead inside the driver itself). That way no > driver needs > to be touched and there are less changes necessary. > And it would change it back to globally-owned strings. Is there any interface in place to do this? Otherwise, tie-ing the string to struct module impacts every single module in the kernel. That's a no-go IMHO and I'm not going to do that. Here's a different proposal: most drivers parse from within module_init, but the parsing could be moved to probe time, when a device struct is available. The allocated memory could then be managed with the devm_ infrastrcuture. This would require to drop the MODULE guards around the options parsing; something we should consider anyway. Gbefb and a few other already do this. See patch 24 for an example. > > With such a change I think the new command parsing functions wouldn't be > needed > either, and ideally, where possible, a conversion to module_param() in > various > places would be better. A number of modules in other subsystems use the same hand-written strsep() loops. So the idea was to use the iterator elsewhere. We could certainly drop it from this patchset, as it's not essential to fixing the memory leak. Best regards Thomas > > I'm sure I haven't overlooked everything yet, but I think if we can reduce > the necessary changes by fixing things globally it makes more sense. > > Helge > >> >> Best regards >> Thomas >> >> Am 09.03.23 um 17:00 schrieb Thomas Zimmermann: >>> Introduce struct option_iter and helpers to parse command-line >>> options with comma-separated key-value pairs. Then convert fbdev >>> drivers to the new interface. Fixes a memory leak in the parsing of >>> the video= option. >>> >>> Before commit 73ce73c30ba9 ("fbdev: Transfer video= option strings to >>> caller; clarify ownership"), a call to fb_get_options() either >>> returned an internal string or a duplicated string; hence ownership of >>> the string's memory buffer was not well defined, but depended on how >>> users specified the video= option on the kernel command line. For >>> global settings, the caller owned the returned memory and for per-driver >>> settings, fb_get_options() owned the memory. As calling drivers were >>> unable to detect the case, they had no option but to leak the the >>> memory. >>> >>> Commit 73ce73c30ba9 ("fbdev: Transfer video= option strings to caller; >>> clarify ownership") changed semantics to caller-owned strings. Drivers >>> still leaked the memory, but at least ownership was clear. >>> >>> This patchset fixes the memory leak and changes string ownership back >>> to fb_get_options(). Patch 1 introduces struct option_iter and a few >>> helpers. The interface takes an option string, such as video=, in the >>> common form value1,key2:value2,value3 etc and returns the individual >>> comma-separated pairs. Various modules use this pattern, so the code >>> is located under lib/. >>> >>> Patches 2 to 100 go through fbdev drivers and convert them to the new >>> interface. This often requires a number of cleanups. A driver would >>> typically refer to the option string's video mode. Such strings are now >>> copied to driver-allocated memory so that drivers don't refer directly >>> to the option string's memory. The option iterator then replaces manual >>> parsing loops based on strsep(","). All driver-allocated memory is >>> released by removing the device or unloading the module. >>> >>> Patch 101 finally changes the ownership of the option string to be >>> internal to fb_get_option(); thereby fixing the memory leak. The option >>> iterator holds its own copy of the string and is not affected by the >>> change. >>> >>> Most fbdev drivers only support to parse option strings if they are >>> built-in. I assume that's because of the original fuzzy semantics of >>> fb_get_options(). A later patchset could change the driver to respect >>> video= settings in any configuration. >>> >>> v2: >>> * use kstrdup()/kfree() for video strings (Geert, Timur) >>> * fix iterator docs (Randy) >>> * update iterator interface >>> >>> Thomas Zimmermann (101): >>> lib: Add option iterator >>> fbdev/68328fb: Remove trailing whitespaces >>> fbdev/68328fb: Remove unused option string >>> fbdev/acornfb: Only init fb_info once >>> fbdev/acornfb: Parse option string with struct option_iter >>> fbdev/amifb: Duplicate video-mode option string >>> fbdev/amifb: Parse option string with struct option_iter >>> fbdev/arkfb: Duplicate video-mode option string >>> fbdev/atafb: Duplicate video-mode option string >>> fbdev/atafb: Parse option string with struct option_iter >>> fbdev/aty: Duplicate video-mode option string >>> fbdev/aty: Parse option string with struct option_iter >>> fbdev/au1100fb: Parse option string with struct option_iter >>> fbdev/au1200fb: Parse option string with struct option_iter >>> fbdev/cirrusfb: Duplicate video-mode option string >>> fbdev/cirrusfb: Parse option string with struct option_iter >>> fbdev/controlfb: Remove trailing whitespaces >>> fbdev/controlfb: Parse option string with struct option_iter >>> fbdev/cyber2000fb: Parse option string with struct option_iter >>> fbdev/efifb: Parse option string with struct option_iter >>> fbdev/fm2fb: Parse option string with struct option_iter >>> fbdev/fsl-diu-fb: Duplicate video-mode option string >>> fbdev/fsl-diu-fb: Parse option string with struct option_iter >>> fbdev/gbefb: Duplicate video-mode option string >>> fbdev/gbefb: Parse option string with struct option_iter >>> fbdev/geode: Duplicate video-mode option string >>> fbdev/geode: Parse option string with struct option_iter >>> fbdev/grvga: Duplicate video-mode option string >>> fbdev/grvga: Parse option string with struct option_iter >>> fbdev/gxt4500: Duplicate video-mode option string >>> fbdev/hyperv_fb: Duplicate video-mode option string >>> fbdev/i740fb: Duplicate video-mode option string >>> fbdev/i740fb: Parse option string with struct option_iter >>> fbdev/i810: Duplicate video-mode option string >>> fbdev/i810: Parse option string with struct option_iter >>> fbdev/imsttfb: Parse option string with struct option_iter >>> fbdev/intelfb: Duplicate video-mode option string >>> fbdev/intelfb: Parse option string with struct option_iter >>> fbdev/imxfb: Duplicate video-mode option string >>> fbdev/imxfb: Parse option string with struct option_iter >>> fbdev/kyrofb: Duplicate video-mode option string >>> fbdev/kyrofb: Parse option string with struct option_iter >>> fbdev/macfb: Remove trailing whitespaces >>> fbdev/macfb: Parse option string with struct option_iter >>> fbdev/matroxfb: Parse option string with struct option_iter >>> fbdev/mx3fb: Duplicate video-mode option string >>> fbdev/mx3fb: Parse option string with struct option_iter >>> fbdev/neofb: Duplicate video-mode option string >>> fbdev/neofb: Parse option string with struct option_iter >>> fbdev/nvidiafb: Duplicate video-mode option string >>> fbdev/nvidiafb: Parse option string with struct option_iter >>> fbdev/ocfb: Duplicate video-mode option string >>> fbdev/ocfb: Parse option string with struct option_iter >>> fbdev/omapfb: Parse option string with struct option_iter >>> fbdev/platinumfb: Remove trailing whitespaces >>> fbdev/platinumfb: Parse option string with struct option_iter >>> fbdev/pm2fb: Duplicate video-mode option string >>> fbdev/pm2fb: Parse option string with struct option_iter >>> fbdev/pm3fb: Duplicate video-mode option string >>> fbdev/pm3fb: Parse option string with struct option_iter >>> fbdev/ps3fb: Duplicate video-mode option string >>> fbdev/ps3fb: Parse option string with struct option_iter >>> fbdev/pvr2fb: Duplicate video-mode option string >>> fbdev/pvr2fb: Parse option string with struct option_iter >>> fbdev/pxafb: Parse option string with struct option_iter >>> fbdev/rivafb: Duplicate video-mode option string >>> fbdev/rivafb: Parse option string with struct option_iter >>> fbdev/s3fb: Duplicate video-mode option string >>> fbdev/s3fb: Parse option string with struct option_iter >>> fbdev/savagefb: Duplicate video-mode option string >>> fbdev/savagefb: Parse option string with struct option_iter >>> fbdev/sisfb: Constify mode string >>> fbdev/sisfb: Parse option string with struct option_iter >>> fbdev/skeletonfb: Parse option string with struct option_iter >>> fbdev/sm712fb: Duplicate video-mode option string >>> fbdev/sstfb: Duplicate video-mode option string >>> fbdev/sstfb: Parse option string with struct option_iter >>> fbdev/stifb: Remove trailing whitespaces >>> fbdev/stifb: Constify option string >>> fbdev/tdfxfb: Duplicate video-mode option string >>> fbdev/tdfxfb: Parse option string with struct option_iter >>> fbdev/tgafb: Duplicate video-mode option string >>> fbdev/tgafb: Parse option string with struct option_iter >>> fbdev/tmiofb: Remove unused option string >>> fbdev/tridentfb: Duplicate video-mode option string >>> fbdev/tridentfb: Parse option string with struct option_iter >>> fbdev/uvesafb: Duplicate video-mode option string >>> fbdev/uvesafb: Parse option string with struct option_iter >>> fbdev/valkyriefb: Remove trailing whitespaces >>> fbdev/valkyriefb: Parse option string with struct option_iter >>> fbdev/vermilion: Remove unused option string >>> fbdev/vesafb: Parse option string with struct option_iter >>> fbdev/vfb: Remove trailing whitespaces >>> fbdev/vfb: Duplicate video-mode option string >>> fbdev/vfb: Parse option string with struct option_iter >>> fbdev/viafb: Parse option string with struct option_iter >>> fbdev/vt8623fb: Duplicate video-mode option string >>> staging/sm750fb: Release g_settings in module-exit function >>> staging/sm750fb: Duplicate video-mode option string >>> staging/sm750fb: Parse option string with struct option_iter >>> fbdev: Constify option strings >>> >>> Documentation/core-api/kernel-api.rst | 9 ++ >>> drivers/staging/sm750fb/sm750.c | 63 ++++---- >>> drivers/video/fbdev/68328fb.c | 24 +-- >>> drivers/video/fbdev/acornfb.c | 23 ++- >>> drivers/video/fbdev/amifb.c | 23 +-- >>> drivers/video/fbdev/arkfb.c | 10 +- >>> drivers/video/fbdev/atafb.c | 21 +-- >>> drivers/video/fbdev/aty/aty128fb.c | 22 ++- >>> drivers/video/fbdev/aty/atyfb_base.c | 23 ++- >>> drivers/video/fbdev/aty/radeon_base.c | 26 +-- >>> drivers/video/fbdev/au1100fb.c | 13 +- >>> drivers/video/fbdev/au1200fb.c | 15 +- >>> drivers/video/fbdev/cirrusfb.c | 30 ++-- >>> drivers/video/fbdev/controlfb.c | 47 +++--- >>> drivers/video/fbdev/core/fb_cmdline.c | 13 +- >>> drivers/video/fbdev/core/modedb.c | 8 +- >>> drivers/video/fbdev/cyber2000fb.c | 17 +- >>> drivers/video/fbdev/efifb.c | 44 ++--- >>> drivers/video/fbdev/ep93xx-fb.c | 2 +- >>> drivers/video/fbdev/fm2fb.c | 14 +- >>> drivers/video/fbdev/fsl-diu-fb.c | 24 +-- >>> drivers/video/fbdev/gbefb.c | 23 +-- >>> drivers/video/fbdev/geode/gx1fb_core.c | 16 +- >>> drivers/video/fbdev/geode/gxfb_core.c | 23 +-- >>> drivers/video/fbdev/geode/lxfb_core.c | 25 +-- >>> drivers/video/fbdev/grvga.c | 18 ++- >>> drivers/video/fbdev/gxt4500.c | 13 +- >>> drivers/video/fbdev/hyperv_fb.c | 18 ++- >>> drivers/video/fbdev/i740fb.c | 26 +-- >>> drivers/video/fbdev/i810/i810_main.c | 26 ++- >>> drivers/video/fbdev/imsttfb.c | 16 +- >>> drivers/video/fbdev/imxfb.c | 21 +-- >>> drivers/video/fbdev/intelfb/intelfbdrv.c | 23 ++- >>> drivers/video/fbdev/kyro/fbdev.c | 21 ++- >>> drivers/video/fbdev/macfb.c | 26 +-- >>> drivers/video/fbdev/matrox/matroxfb_base.c | 19 +-- >>> drivers/video/fbdev/mx3fb.c | 23 ++- >>> drivers/video/fbdev/neofb.c | 26 +-- >>> drivers/video/fbdev/nvidia/nvidia.c | 26 ++- >>> drivers/video/fbdev/ocfb.c | 21 ++- >>> drivers/video/fbdev/omap/omapfb_main.c | 15 +- >>> drivers/video/fbdev/platinumfb.c | 44 ++--- >>> drivers/video/fbdev/pm2fb.c | 25 +-- >>> drivers/video/fbdev/pm3fb.c | 27 ++-- >>> drivers/video/fbdev/ps3fb.c | 28 ++-- >>> drivers/video/fbdev/pvr2fb.c | 32 ++-- >>> drivers/video/fbdev/pxafb.c | 18 ++- >>> drivers/video/fbdev/riva/fbdev.c | 26 ++- >>> drivers/video/fbdev/s3fb.c | 27 ++-- >>> drivers/video/fbdev/savage/savagefb_driver.c | 20 ++- >>> drivers/video/fbdev/sis/sis_main.c | 24 +-- >>> drivers/video/fbdev/skeletonfb.c | 17 +- >>> drivers/video/fbdev/sm712fb.c | 12 +- >>> drivers/video/fbdev/sstfb.c | 25 +-- >>> drivers/video/fbdev/stifb.c | 162 +++++++++---------- >>> drivers/video/fbdev/tdfxfb.c | 21 ++- >>> drivers/video/fbdev/tgafb.c | 30 ++-- >>> drivers/video/fbdev/tmiofb.c | 24 +-- >>> drivers/video/fbdev/tridentfb.c | 27 ++-- >>> drivers/video/fbdev/uvesafb.c | 21 ++- >>> drivers/video/fbdev/valkyriefb.c | 30 ++-- >>> drivers/video/fbdev/vermilion/vermilion.c | 7 +- >>> drivers/video/fbdev/vesafb.c | 16 +- >>> drivers/video/fbdev/vfb.c | 35 ++-- >>> drivers/video/fbdev/via/viafbdev.c | 15 +- >>> drivers/video/fbdev/vt8623fb.c | 11 +- >>> include/linux/cmdline.h | 36 +++++ >>> include/linux/fb.h | 2 +- >>> lib/Makefile | 2 +- >>> lib/cmdline_iter.c | 109 +++++++++++++ >>> 70 files changed, 1087 insertions(+), 682 deletions(-) >>> create mode 100644 include/linux/cmdline.h >>> create mode 100644 lib/cmdline_iter.c >>> >> > -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Ivo Totev