From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.0 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id D380EC433E0 for ; Mon, 3 Aug 2020 19:33:18 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 487A822B45 for ; Mon, 3 Aug 2020 19:33:18 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="oLRC8NpJ"; dkim=fail reason="signature verification failed" (2048-bit key) header.d=baylibre-com.20150623.gappssmtp.com header.i=@baylibre-com.20150623.gappssmtp.com header.b="jSmTnJX1" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 487A822B45 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=baylibre.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:To:Subject:Message-ID:Date:From:In-Reply-To: References:MIME-Version:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=2pb7U59sAmGWw3BHMpeV962nHoFvTYpKLrUFpmLzVd0=; b=oLRC8NpJK+xWWOnVGdLusmMH9 Llc4ItAFnUpUVQL6JaMg3/XWN2oHiu2NAdy3LhZJvc22ZNqHUAtcpnZcdEuLoxJ3YO7T2WNUxwQ6W FjJJm+qqYgf1gRt58CQ11GQTvFKQmVIg9vRU4BTtNVnXWQGtzXHofySGG+WC5iQFe6a4vVGcx+wvN 15zsZdoy4hkNmCzxqL6OaZysXMs7TZJPXH0gTVgPbK6GKdCXVzDmVxCBqTIInQJ8N4ULE4VhEBaWw kY8jyKtj7UolDz7fAj+aAnRd0vLfrVeLG/ZeudeT9FKDDdIWyZj6/U9YYcsRlBqiYD2BpTh/gHKZx ZgYrycdpQ==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1k2gBl-0003ND-8R; Mon, 03 Aug 2020 19:32:01 +0000 Received: from mail-qt1-x843.google.com ([2607:f8b0:4864:20::843]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1k2gBh-0003KE-FX for linux-arm-kernel@lists.infradead.org; Mon, 03 Aug 2020 19:31:58 +0000 Received: by mail-qt1-x843.google.com with SMTP id w9so29119859qts.6 for ; Mon, 03 Aug 2020 12:31:55 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=baylibre-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=RvzegeXfDF+FVdvZWpUY2C/JYcGtzK0e5Km4cAoPNN8=; b=jSmTnJX12hJqDm4wSS6o7mGjoyl/+MvIhk0XVP7uEzSk09YcYc0nfbe7zl7aGuS6wC SZGNqDDIANha72buny9X6caEYisJ0XYhId0is+ZwGaTqeHILDvgTJPVKJvBN+9ZYXvZP 0/nufiyHW+XAHqa5hL6CByacEB0uNBoHAuc4Kis0WamHtXH5CUsWqOiezhW9OIyk69+N oPiQemIU1078XmA59msE3KdOqcmq1SdKyAOJGFHHKvknx5m21uk7o6pQX+nWrZAsH1Oj YYTBVnF2EOEmJM9paMatW2pjQOsHClWl2IDuqTaFsRERReRg3AsBTwZj2m1vAcJQ5fLE ctSw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=RvzegeXfDF+FVdvZWpUY2C/JYcGtzK0e5Km4cAoPNN8=; b=szOhzQK3ZnM1kS3yYSzA6l2qp3UwtKFUodEvifPmbGTA/2AamklGhwGPCO/BkOIkcm RPcyBOMo8nQdk0FvsEjbxFvJcEoMbuHDhRFXc1pl5efJr4PK8+PNb7xtQmsbgWIURSM4 c9TbpY8BmgtCEuPCcmN1TM/hAeZrHSEnXC2ME1vQHLkUaP0lj2wVlSOo/f/BtN8b+Mif 2aV/bQ0iw8ykT66/6V1LcS4YCKclRwi8tI0QbQ7f8QmUBM0e2tDn7JX8uLyrajSlYwN2 1jiIRleEo7vz2y04lnVZRoY0EOMaNSe2yA7vntsssvgBCgnoqNz7xRbdgHgXdg6j5a0c NPLQ== X-Gm-Message-State: AOAM533xu4jHoZiqnhNeO1SGv/DQSgFxQcic/N5MyYxFFEzR/X6+umaT RYedCjKF45895AN65kn4pBRw87vh7YQdiNsFCYkYQIGj X-Google-Smtp-Source: ABdhPJyAJVak0FjVUTJi1mcLTSkaBZWjBpWRxzk7IIBX2OckQU/gStJWBE/N9Odcq/m1cUmnKTfpgZfhgTWyUcthDaU= X-Received: by 2002:ac8:72cc:: with SMTP id o12mr17678716qtp.27.1596483113936; Mon, 03 Aug 2020 12:31:53 -0700 (PDT) MIME-Version: 1.0 References: <20200802083458.24323-1-brgl@bgdev.pl> <20200802083458.24323-2-brgl@bgdev.pl> In-Reply-To: From: Bartosz Golaszewski Date: Mon, 3 Aug 2020 21:31:42 +0200 Message-ID: Subject: Re: [PATCH v6 1/3] devres: provide devm_krealloc() To: Andy Shevchenko X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200803_153157_571343_B5D8C94F X-CRM114-Status: GOOD ( 30.06 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: linux-arm Mailing List , Lars-Peter Clausen , linux-iio , Greg Kroah-Hartman , Bartosz Golaszewski , Michal Simek , Linux Kernel Mailing List , Guenter Roeck , Peter Meerwald-Stadler , Hartmut Knaack , Andy Shevchenko , Jonathan Cameron Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Sun, Aug 2, 2020 at 12:42 PM Andy Shevchenko wrote: > [snip] > > I was thinking about this bit... Shouldn't we rather issue a simple > dev_warn() and return the existing pointer? > For example in some cases we might want to have resources coming > either from heap or from constant. Then, if at some circumstances we > would like to extend that memory (only for non-constant cases) we > would need to manage this ourselves. Otherwise we may simply call > krealloc(). > It seems that devm_kstrdup_const returns an initial pointer. Getting > NULL is kinda inconvenient (and actually dev_warn() might also be > quite a noise, however I would give a message to the user, because > it's something worth checking). > But this is inconsistent behavior: if you pass a pointer to ro memory to devm_krealloc() it will not resize it but by returning a valid pointer it will make you think it did -> you end up writing to ro memory in good faith. > ... > > > + spin_lock_irqsave(&dev->devres_lock, flags); > > + old_dr = find_dr(dev, devm_kmalloc_release, devm_kmalloc_match, ptr); > > + spin_unlock_irqrestore(&dev->devres_lock, flags); > > > + if (!old_dr) { > > I would have this under spin lock b/c of below. > > > + WARN(1, "Memory chunk not managed or managed by a different device."); > > + return NULL; > > + } > > > + old_head = old_dr->node.entry; > > This would be still better to be under spin lock. > > > + new_dr = krealloc(old_dr, total_size, gfp); > > + if (!new_dr) > > + return NULL; > > And perhaps spin lock taken already here. > > > + if (new_dr != old_dr) { > > + spin_lock_irqsave(&dev->devres_lock, flags); > > + list_replace(&old_head, &new_dr->node.entry); > > + spin_unlock_irqrestore(&dev->devres_lock, flags); > > + } > > Yes, I understand that covering more code under spin lock does not fix > any potential race, but at least it minimizes scope of the code that > is not under it to see exactly what is problematic. > > I probably will think more about a better approach to avoid potential races. My thinking behind this was this: we already have users who call devres_find() and do something with the retrieved resources without holding the devres_lock - so it's assumed that users are sane enough to not be getting in each other's way. Now I see that the difference is that here we're accessing the list node and it can change if another thread is adding a different devres to the same device. So this should definitely be protected somehow. I think that we may have to give up using real krealloc() and instead just reimplement its behavior in the following way: Still outside of spinlock check if the new total size is smaller or equal to the previous one. If so: return the same pointer. If not: allocate a new devres as if it were for devm_kmalloc() but don't add it to the list yet. Take the spinlock - check if we can find the devres - if not: kfree() the new and old chunk and return NULL. If yes: copy the contents of the devres node into the new chunk as well as the memory contents. Replace the old one on the list and free it. Release spinlock and return. Does that work? Bart _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel