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=-5.9 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE, SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 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 0C4A2C4708F for ; Wed, 2 Jun 2021 09:00:23 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id DD1F1613D6 for ; Wed, 2 Jun 2021 09:00:22 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230093AbhFBJCD (ORCPT ); Wed, 2 Jun 2021 05:02:03 -0400 Received: from szxga01-in.huawei.com ([45.249.212.187]:2845 "EHLO szxga01-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229754AbhFBJCB (ORCPT ); Wed, 2 Jun 2021 05:02:01 -0400 Received: from dggeme759-chm.china.huawei.com (unknown [172.30.72.57]) by szxga01-in.huawei.com (SkyGuard) with ESMTP id 4Fw2tF6fY4zWqMK; Wed, 2 Jun 2021 16:55:33 +0800 (CST) Received: from [127.0.0.1] (10.40.188.144) by dggeme759-chm.china.huawei.com (10.3.19.105) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.2176.2; Wed, 2 Jun 2021 17:00:16 +0800 Subject: Re: [PATCH 1/2] topology: use bin_attribute to avoid buff overflow To: Andy Shevchenko CC: Greg KH , Tian Tao , Linux Kernel Mailing List , Andrew Morton , Barry Song , Andy Shevchenko , "Rafael J. Wysocki" , Jonathan Cameron References: <1622516210-10886-1-git-send-email-tiantao6@hisilicon.com> <1622516210-10886-2-git-send-email-tiantao6@hisilicon.com> <4c9c7c17-e8d1-d601-6262-8064293a06a9@huawei.com> From: "tiantao (H)" Message-ID: Date: Wed, 2 Jun 2021 17:00:16 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101 Thunderbird/60.8.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.40.188.144] X-ClientProxiedBy: dggems703-chm.china.huawei.com (10.3.19.180) To dggeme759-chm.china.huawei.com (10.3.19.105) X-CFilter-Loop: Reflected Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 在 2021/6/2 16:48, Andy Shevchenko 写道: > On Wed, Jun 2, 2021 at 9:45 AM tiantao (H) wrote: >> 在 2021/6/2 14:18, Greg KH 写道: >>> On Wed, Jun 02, 2021 at 02:14:49PM +0800, tiantao (H) wrote: >>>> 在 2021/6/1 12:58, Greg KH 写道: >>>>> On Tue, Jun 01, 2021 at 10:56:49AM +0800, Tian Tao wrote: > ... > >>>>>> /** >>>>>> + * bitmap_print_to_buf - convert bitmap to list or hex format ASCII string >>>>>> + * @list: indicates whether the bitmap must be list >>>>>> + * @buf: page aligned buffer into which string is placed >>>>>> + * @maskp: pointer to bitmap to convert >>>>>> + * @nmaskbits: size of bitmap, in bits >>>>>> + * @off: offset in buf >>>>>> + * @count: count that already output >>>>>> + * >>>>>> + * the role of bitmap_print_to_buf and bitmap_print_to_pagebuf is >>>>>> + * the same, the difference is that the second parameter of >>>>>> + * bitmap_print_to_buf can be more than one pagesize. >>>>>> + */ >>>>>> +int bitmap_print_to_buf(bool list, char *buf, const unsigned long *maskp, >>>>>> + int nmaskbits, loff_t off, size_t count) >>>>>> +{ >>>>>> + int len, size; >>>>>> + void *data; >>>>>> + char *fmt = list ? "%*pbl\n" : "%*pb\n"; >>>>>> + >>>>>> + len = snprintf(NULL, 0, fmt, nmaskbits, maskp); >>>>>> + >>>>>> + data = kvmalloc(len+1, GFP_KERNEL); >>>>>> + if (!data) >>>>>> + return -ENOMEM; >>>>>> + >>>>>> + size = scnprintf(data, len+1, fmt, nmaskbits, maskp); >>>>>> + size = memory_read_from_buffer(buf, count, &off, data, size); >>>>>> + kvfree(data); >>>>>> + >>>>>> + return size; >>>>> Why is this so different from bitmap_print_to_pagebuf()? Can't you just >>>>> use this function as the "real" function and then change >>>>> bitmap_print_to_pagebuf() to call it with a size of PAGE_SIZE? >>>> Do you mean do following change, is that correct? :-) >>> Maybe, it is whitespace corrupted, and it still feels like this function >>> is much bigger than it needs to be given the function it is replacing is >>> only a simple sprintf() call. >>> >>>> +int bitmap_print_to_buf(bool list, char *buf, const unsigned long *maskp, >>>> + int nmaskbits, loff_t off, size_t count) >>>> +{ >>>> + int len, size; >>>> + void *data; >>>> + const char *fmt = list ? "%*pbl\n" : "%*pb\n"; >>>> + >>>> + if (off == LLONG_MAX && count == PAGE_SIZE - offset_in_page(buf)) >>>> + return scnprintf(buf, count, fmt, nmaskbits, maskp); >>>> + >>>> + len = snprintf(NULL, 0, fmt, nmaskbits, maskp); >>>> + >>>> + data = kvmalloc(len+1, GFP_KERNEL); >>> Why do you need to allocate more memory? And why kvmalloc()? >> Because the memory here will exceed a pagesize and we don't know the >> exact size, we have to call >> >> snprintf first to get the actual size. kvmalloc() is used because when >> physical memory is tight, kmalloc >> >> may fail, but vmalloc will succeed. It is not so bad that the memory is >> not requested here. > To me it sounds like the function is overengineered / lacks thought > through / optimization. > Can you provide a few examples that require the above algorithm? so you think we should use kmalloc instead of kvmalloc ? > >>>> + if (!data) >>>> + return -ENOMEM; >>>> + >>>> + size = scnprintf(data, len+1, fmt, nmaskbits, maskp); >>>> + >>>> + size = memory_read_from_buffer(buf, count, &off, data, size); >>>> + kvfree(data); >>>> + >>>> + return size; >>>> +} > > -- > With Best Regards, > Andy Shevchenko > . >