From patchwork Tue Mar 25 10:44:02 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jani Nikula X-Patchwork-Id: 3886771 Return-Path: X-Original-To: patchwork-dri-devel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 884929F2E8 for ; Tue, 25 Mar 2014 10:44:01 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id ADD7E2021C for ; Tue, 25 Mar 2014 10:44:00 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by mail.kernel.org (Postfix) with ESMTP id B70CC2020E for ; Tue, 25 Mar 2014 10:43:59 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 140366E14B; Tue, 25 Mar 2014 03:43:59 -0700 (PDT) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by gabe.freedesktop.org (Postfix) with ESMTP id E14976E14B for ; Tue, 25 Mar 2014 03:43:57 -0700 (PDT) Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga101.fm.intel.com with ESMTP; 25 Mar 2014 03:43:38 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="4.97,728,1389772800"; d="scan'208";a="506160838" Received: from jnikula-mobl1.fi.intel.com (HELO localhost) ([10.237.72.185]) by fmsmga002.fm.intel.com with ESMTP; 25 Mar 2014 03:43:37 -0700 From: Jani Nikula To: Daniel Vetter , Dmitry Malkin Subject: Re: [DRM] drm_get_connector_name internal static string buffer causes a race In-Reply-To: <20140324203132.GK26878@phenom.ffwll.local> Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo References: <645ee6e22cad47d38a2b35c21c8d5fe3@DC1-MBX-01.ptsecurity.ru> <20140324203132.GK26878@phenom.ffwll.local> User-Agent: Notmuch/0.17+152~g1782a89fe6da (http://notmuchmail.org) Emacs/23.3.1 (x86_64-pc-linux-gnu) Date: Tue, 25 Mar 2014 12:44:02 +0200 Message-ID: <87k3biimml.fsf@intel.com> MIME-Version: 1.0 Cc: "dri-devel@lists.freedesktop.org" X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" X-Spam-Status: No, score=-4.6 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_MED, RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Mon, 24 Mar 2014, Daniel Vetter wrote: > On Mon, Mar 24, 2014 at 12:06:21PM +0000, Dmitry Malkin wrote: >> >> Hello guys, >> >> I've been playing with reloading intel gfx driver (i915) in a cycle, >> for a while, and at some point I've found a non-deterministic kernel >> crash with a highly-variable iteration dependency -- 2 to 200 driver >> reload iterations. >> >> The apparent race is over the shared internal string buffer in >> drm_get_connector_name(). It is mostly harmless, due to its results >> being mostly used for log output, but in at least one case -- >> drm_sysfs_connector_add() -- this leads to a more critical error. >> >> Race scenario: >> >> - drm_sysfs_connector_add() >> - drm_get_connector_name() >> vs. >> - anything that generates log messages involving DRM connectors >> - drm_get_connector_name() >> >> (and many other from drm_crtc.c) shares with caller const char* to >> internal static char buffer. If something call it from other thread, >> while main thread strore and use returned pointer it may overwrite >> connector name. >> >> Here are we go: registering HDMI connecter (drm_sysfs_connector_add >> store and use pointer from drm_get_connector_name) and the same time >> got VGA connector name down through the stack. (the second thread is >> upowerd who watch continuously sysfs) > > Yeah, in my recent kerneldoc series I've noticed this too and added FIXME > comments. There's a lot more than just those you've pointed out. The > problem is that fixing these will be an awful lot of work since you need > to add proper cleanup code (calling kfree()) to all the required places. > > So a full subsystem wide code audit for every single use-site of these. It would be much easier and much much less error prone to add a name field (either a fixed size or kmalloced buffer) in drm_connector and drm_encoder structs, and initialize them in connector/encoder init. AFAICT the name does not change during the objects' lifetime. None of the use-sites would need to be changed. The question is whether (number of connectors + number of encoders) * 32 bytes is too high a price to pay for the implementation simplicity. I think not. Another alternative to returning kmalloced strings is: So the callers may use the kind of buffers they want. Something like this may be needed for drm_get_format_name() anyway. BR, Jani. diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 16ca28ed5ee8..85c8ed191daa 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -277,11 +277,10 @@ EXPORT_SYMBOL(drm_get_encoder_name); * * FIXME: This isn't really multithreading safe. */ -const char *drm_get_connector_name(const struct drm_connector *connector) +const char *drm_get_connector_name(char *buf, size_t bufsize, + const struct drm_connector *connector) { - static char buf[32]; - - snprintf(buf, 32, "%s-%d", + snprintf(buf, bufsize, "%s-%d", drm_connector_enum_list[connector->connector_type].name, connector->connector_type_id); return buf;