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=-3.7 required=3.0 tests=BAYES_00,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 C3EE7C433DF for ; Mon, 17 Aug 2020 20:02:30 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 915EA204EC for ; Mon, 17 Aug 2020 20:02:30 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=bgdev-pl.20150623.gappssmtp.com header.i=@bgdev-pl.20150623.gappssmtp.com header.b="UHRcBPsn" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729378AbgHQUCU (ORCPT ); Mon, 17 Aug 2020 16:02:20 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:48278 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1729317AbgHQUCR (ORCPT ); Mon, 17 Aug 2020 16:02:17 -0400 Received: from mail-il1-x142.google.com (mail-il1-x142.google.com [IPv6:2607:f8b0:4864:20::142]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 9C406C061342 for ; Mon, 17 Aug 2020 13:02:17 -0700 (PDT) Received: by mail-il1-x142.google.com with SMTP id r13so11206887iln.0 for ; Mon, 17 Aug 2020 13:02:17 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bgdev-pl.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=42EHKaiZokDYEpY04AQEHHdjVaoqLPYqnG7cOPFCkVo=; b=UHRcBPsnuwQhn3lAwxUnaMePKdIl5grFxltDU8PhTR6WLHfWFDbi2EKC+nzAhXFKsk Kj26U6KLk9VROr9Zvg9QT+0ePSN5fZsV5DJBIov3SJk+OVUohgcEBBDoYt/AdOFb3VId ntKhlpjtfnSoZfherw1s1yh05Ozxy1lNa20PIXZN4EYEu7T8EEsf4PWtydabRxXHX8fx lqd/l6hGDUi/rFf9nKBX1AKe0Vo3ohnAoa2gM4PZ/3byBobTwWlN+6PmI9PFlph0wyDG BoG6ayyGj/c0QIyGXj91qlxT7SaGZrldW0BhTH39V4+UPxdEJMrqCbBFSFf0+VUc9MHf OTGA== 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=42EHKaiZokDYEpY04AQEHHdjVaoqLPYqnG7cOPFCkVo=; b=ZWb/GHRbJyuk0UN5IKnSuhvXlXwrlXJOpyjGo7IemyMuBGfWkz54d129xU2l/Q4whv d+jQ9IzOO0y/YM2Dh0lB6s58zg7hf5bdaPVj0VKbU8N8YzLVwl5GqjWVlFj9QR2VPIpc 0wrYmX65irIXcWA5Yyukus06gNhPX/XOk+xdfO/QzR6hNXydK1JtznOAuW3n2WG3y9Li Q4rH4U/Ks8U/mymOkC92tXGilcO30eBW+AQyBzku5nO3sC05lYAukCPsBohbymqYVVY3 SujnARXDBw7+YkFSTW4QoNlmBl4rhmDgXrNDIhvhJjQtDVGnYFGCJsyCfyaR4UjjkWEU rX8g== X-Gm-Message-State: AOAM533BQb6FclcoYG2RPR3nh1GUxSjoBsh8NlH3lroxGXYKEQfLZrca 0bsRwvSnWT7CUQk2EVE3di70Sn8nBTLRTy3a+80s7A== X-Google-Smtp-Source: ABdhPJz7+/cSad+TqEVktg+PR5xxDaQJGXqAXQEodbtGBY1kqiUkr3yUdwTHkmeHwjQMPc5WO3+3DORBrxf8XAicLVM= X-Received: by 2002:a92:a157:: with SMTP id v84mr14515417ili.189.1597694536738; Mon, 17 Aug 2020 13:02:16 -0700 (PDT) MIME-Version: 1.0 References: <20200817170535.17041-1-brgl@bgdev.pl> <20200817170535.17041-2-brgl@bgdev.pl> <20200817173908.GS1891694@smile.fi.intel.com> In-Reply-To: <20200817173908.GS1891694@smile.fi.intel.com> From: Bartosz Golaszewski Date: Mon, 17 Aug 2020 22:02:05 +0200 Message-ID: Subject: Re: [PATCH v7 1/3] devres: provide devm_krealloc() To: Andy Shevchenko Cc: Jonathan Cameron , Hartmut Knaack , Lars-Peter Clausen , Peter Meerwald-Stadler , Michal Simek , Greg Kroah-Hartman , Guenter Roeck , linux-iio , Linux ARM , Linux Kernel Mailing List , Bartosz Golaszewski Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Aug 17, 2020 at 7:43 PM Andy Shevchenko wrote: > > On Mon, Aug 17, 2020 at 07:05:33PM +0200, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski > > > > Implement the managed variant of krealloc(). This function works with > > all memory allocated by devm_kmalloc() (or devres functions using it > > implicitly like devm_kmemdup(), devm_kstrdup() etc.). > > > > Managed realloc'ed chunks can be manually released with devm_kfree(). > > Thanks for an update! My comments / questions below. > > ... > > > +static struct devres *to_devres(void *data) > > +{ > > + return (struct devres *)((u8 *)data - ALIGN(sizeof(struct devres), > > + ARCH_KMALLOC_MINALIGN)); > > Do you really need both explicit castings? > Yeah, we can probably drop the (struct devres *) here. > > +} > > ... > > > + total_old_size = ksize(to_devres(ptr)); > > But how you can guarantee this pointer: > - belongs to devres, We can only check if a chunk is dynamically allocated with ksize() - it will return 0 if it isn't and I'll add a check for that in the next iteration. We check whether it's a managed chunk later after taking the lock. > - hasn't gone while you run a ksize()? > At some point you need to draw a line. In the end: how do you guarantee a devres buffer hasn't been freed when you're using it? In my comment to the previous version of this patch I clarified that we need to protect all modifications of the devres linked list - we must not realloc a chunk that contains the links without taking the spinlock but also we must not call alloc() funcs with GFP_KERNEL with spinlock taken. The issue we could run into is: someone modifies the linked list by adding/removing other managed resources, not modifying this one. The way this function works now guarantees it but other than that: it's up to the users to not free memory they're actively using. > ... > > > + new_dr = alloc_dr(devm_kmalloc_release, > > + total_new_size, gfp, dev_to_node(dev)); > > Can you move some parameters to the previous line? > Why though? It's fine this way. > > + if (!new_dr) > > + return NULL; > > ... > > > + spin_lock_irqsave(&dev->devres_lock, flags); > > + > > + old_dr = find_dr(dev, devm_kmalloc_release, devm_kmalloc_match, ptr); > > + if (!old_dr) { > > + spin_unlock_irqrestore(&dev->devres_lock, flags); > > + devres_free(new_dr); > > + WARN(1, "Memory chunk not managed or managed by a different device."); > > + return NULL; > > + } > > + > > + replace_dr(dev, &old_dr->node, &new_dr->node); > > + > > + spin_unlock_irqrestore(&dev->devres_lock, flags); > > + > > + memcpy(new_dr->data, old_dr->data, devres_data_size(total_old_size)); > > But new_dr may concurrently gone at this point, no? It means memcpy() should be > under spin lock. > Just as I explained above: we're protecting the linked list, not the resource itself. Bartosz 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=-3.8 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 A907CC433DF for ; Mon, 17 Aug 2020 20:03:47 +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 6C874204EC for ; Mon, 17 Aug 2020 20:03:47 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="Faxq9bVL"; dkim=fail reason="signature verification failed" (2048-bit key) header.d=bgdev-pl.20150623.gappssmtp.com header.i=@bgdev-pl.20150623.gappssmtp.com header.b="UHRcBPsn" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 6C874204EC Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=bgdev.pl 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=pEMUxgcGIJuXp98Umqpa5H0tVkhQolUassxEvstQ3k4=; b=Faxq9bVL4sLEATu+zlXcfrFGu F9H70JRgaEukSIDNknOJH9FBXWV82PJ5V0YgXQ1aRhNlILtnWJaqQKFLe0Ws9ZHSpoY5t2VJEst8P hhcPeabNU6Y/fNBBHlekQ+5l/sz6ASJ7TUDt0JCK1zZm1hr+Ke3qaRkkflpPz1QCF9iYK0WK2o/Ec v0w/jjddRoYI3BPQBwwyXywwYL3PhJfuZ2LzfPYxpnwkVE+MOEO5PNI/u37NX7OUvBTiWqHa+vuWY Obl6ou9TOflr4obKKuBTZkwGJVxm0eMWnIJbd5uNn0pSfRu52D2TqytmjsdPC3KhP+IwkAJrKS0yq 4HsP6V7eg==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1k7lKn-0004sd-2V; Mon, 17 Aug 2020 20:02:21 +0000 Received: from mail-il1-x141.google.com ([2607:f8b0:4864:20::141]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1k7lKk-0004sG-KA for linux-arm-kernel@lists.infradead.org; Mon, 17 Aug 2020 20:02:19 +0000 Received: by mail-il1-x141.google.com with SMTP id z3so15643515ilh.3 for ; Mon, 17 Aug 2020 13:02:18 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bgdev-pl.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=42EHKaiZokDYEpY04AQEHHdjVaoqLPYqnG7cOPFCkVo=; b=UHRcBPsnuwQhn3lAwxUnaMePKdIl5grFxltDU8PhTR6WLHfWFDbi2EKC+nzAhXFKsk Kj26U6KLk9VROr9Zvg9QT+0ePSN5fZsV5DJBIov3SJk+OVUohgcEBBDoYt/AdOFb3VId ntKhlpjtfnSoZfherw1s1yh05Ozxy1lNa20PIXZN4EYEu7T8EEsf4PWtydabRxXHX8fx lqd/l6hGDUi/rFf9nKBX1AKe0Vo3ohnAoa2gM4PZ/3byBobTwWlN+6PmI9PFlph0wyDG BoG6ayyGj/c0QIyGXj91qlxT7SaGZrldW0BhTH39V4+UPxdEJMrqCbBFSFf0+VUc9MHf OTGA== 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=42EHKaiZokDYEpY04AQEHHdjVaoqLPYqnG7cOPFCkVo=; b=BrZGg/w97fYBXQuzGGEKU96S5bEOdRQLXYnBeXaHplEhMspEQXENpCDQGMXueQa2jT Fc9JNAdjv2gbyXISCJePSNNSK52Ct2kCAQfC7VWMcMZGVFCANQAsbq6cfTibhK+ajEUz UzyXFfy/y4oWd6gV90fWZYaFDAUW1VYfeVMkvPrCZgYs1Pom2VH+eJlgR6IlS90TsykB SjvuHKaiNZdesjLWki7tvfbJhVoMuRi+erUEG84HN8qjOyQkG+qXo8kO8TtjA8r2A1dn 87M23ZiAz7NJ4XhKNf/bOGAqTwK+gVNpUInwZEBZFdcdRX8wt8jlv+DFC4kAXtFJhz+f 45aQ== X-Gm-Message-State: AOAM531D5nes3wMmTpFcoBMh2t6KpE53fhcx79/m61YPoIO74dMBkA0j D9WT6tRqg/UYa7YszResoBuhL42WoNlZdU10IMghfQ== X-Google-Smtp-Source: ABdhPJz7+/cSad+TqEVktg+PR5xxDaQJGXqAXQEodbtGBY1kqiUkr3yUdwTHkmeHwjQMPc5WO3+3DORBrxf8XAicLVM= X-Received: by 2002:a92:a157:: with SMTP id v84mr14515417ili.189.1597694536738; Mon, 17 Aug 2020 13:02:16 -0700 (PDT) MIME-Version: 1.0 References: <20200817170535.17041-1-brgl@bgdev.pl> <20200817170535.17041-2-brgl@bgdev.pl> <20200817173908.GS1891694@smile.fi.intel.com> In-Reply-To: <20200817173908.GS1891694@smile.fi.intel.com> From: Bartosz Golaszewski Date: Mon, 17 Aug 2020 22:02:05 +0200 Message-ID: Subject: Re: [PATCH v7 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-20200817_160218_830195_6C7BB595 X-CRM114-Status: GOOD ( 26.40 ) 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 , Lars-Peter Clausen , linux-iio , Greg Kroah-Hartman , Michal Simek , Linux Kernel Mailing List , Bartosz Golaszewski , Guenter Roeck , Peter Meerwald-Stadler , Hartmut Knaack , 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 Mon, Aug 17, 2020 at 7:43 PM Andy Shevchenko wrote: > > On Mon, Aug 17, 2020 at 07:05:33PM +0200, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski > > > > Implement the managed variant of krealloc(). This function works with > > all memory allocated by devm_kmalloc() (or devres functions using it > > implicitly like devm_kmemdup(), devm_kstrdup() etc.). > > > > Managed realloc'ed chunks can be manually released with devm_kfree(). > > Thanks for an update! My comments / questions below. > > ... > > > +static struct devres *to_devres(void *data) > > +{ > > + return (struct devres *)((u8 *)data - ALIGN(sizeof(struct devres), > > + ARCH_KMALLOC_MINALIGN)); > > Do you really need both explicit castings? > Yeah, we can probably drop the (struct devres *) here. > > +} > > ... > > > + total_old_size = ksize(to_devres(ptr)); > > But how you can guarantee this pointer: > - belongs to devres, We can only check if a chunk is dynamically allocated with ksize() - it will return 0 if it isn't and I'll add a check for that in the next iteration. We check whether it's a managed chunk later after taking the lock. > - hasn't gone while you run a ksize()? > At some point you need to draw a line. In the end: how do you guarantee a devres buffer hasn't been freed when you're using it? In my comment to the previous version of this patch I clarified that we need to protect all modifications of the devres linked list - we must not realloc a chunk that contains the links without taking the spinlock but also we must not call alloc() funcs with GFP_KERNEL with spinlock taken. The issue we could run into is: someone modifies the linked list by adding/removing other managed resources, not modifying this one. The way this function works now guarantees it but other than that: it's up to the users to not free memory they're actively using. > ... > > > + new_dr = alloc_dr(devm_kmalloc_release, > > + total_new_size, gfp, dev_to_node(dev)); > > Can you move some parameters to the previous line? > Why though? It's fine this way. > > + if (!new_dr) > > + return NULL; > > ... > > > + spin_lock_irqsave(&dev->devres_lock, flags); > > + > > + old_dr = find_dr(dev, devm_kmalloc_release, devm_kmalloc_match, ptr); > > + if (!old_dr) { > > + spin_unlock_irqrestore(&dev->devres_lock, flags); > > + devres_free(new_dr); > > + WARN(1, "Memory chunk not managed or managed by a different device."); > > + return NULL; > > + } > > + > > + replace_dr(dev, &old_dr->node, &new_dr->node); > > + > > + spin_unlock_irqrestore(&dev->devres_lock, flags); > > + > > + memcpy(new_dr->data, old_dr->data, devres_data_size(total_old_size)); > > But new_dr may concurrently gone at this point, no? It means memcpy() should be > under spin lock. > Just as I explained above: we're protecting the linked list, not the resource itself. Bartosz _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel