Received: by 10.223.185.116 with SMTP id b49csp940346wrg; Wed, 14 Feb 2018 09:10:24 -0800 (PST) X-Google-Smtp-Source: AH8x227dhIRkt52McxZb90XevZomHcVhFBRO4kI06HsjRunFNaRemV5EfjqjS1O+/oNU8f2lQpKD X-Received: by 2002:a17:902:9343:: with SMTP id g3-v6mr5162903plp.319.1518628224449; Wed, 14 Feb 2018 09:10:24 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1518628224; cv=none; d=google.com; s=arc-20160816; b=GZ02A+roE3KArLSx3I3aPVlmIpeyRHdigyAJfB6aoOvNy3rd0bnrKIwHntRWOHjycX TWcpq3TXypKZbVTWNxaliyEb61HzRI30A5HinUVR2BCGQqQnS9yNZR0mBx89LCOWD/Gc hRyLIIPapL0wSccsaKEtHCKZBZpwZA+qLMCkSTMkY2ABxkx6Mr60HIhNR3M3FDOEvWz6 JVYQwh1Y/R276kboVGJdoNnHbobTL37Yy598aCZxNCyr9X0cvxuSkv2RxNlTleebM051 qXK03mObJs8/TDXGIO9qAQvT+gBWMBc0bC1Mcr3V50fm8Vh/LKejtC1iXmBCF2JR5wZg Hmlw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:cc:to:subject:message-id:date:from :references:in-reply-to:mime-version:arc-authentication-results; bh=Bnp1+jyOuK7EGJG8HfpD7bdXpiKT7WdGkHn+DtoBccg=; b=PqzedZfeSr70U+omFkzn6gekGcVg2FSIciCGjYltcC6PFyN7EB2/nbHo3I2PBiu4Vl hN8/lnO89XQ09RxwJQEYx4QPGB0TF/rhFLG4Qzi+IfLUlkl+iJ6Wx/mKqUwt9Fva7Rqg FaMl3XQJQcv8kw+QTTYxmp7QG1ZuwkAzk1OVd5XFMfSQiMuHHYv09qC59AScMUgb3MV+ 9/yZxNXw2kOcpKZzh3gl38fWh+yxeKHWvFrIdxZQ2t+1bepkTdYEXl9X+DMWoKwm46EE 5Vq60BgdGj74Cs5I9HqQPcQMyu0erHJqbvpQ/9OAuodDyieMY7zYu58IVwN7Wglxi5Cq HdZQ== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=redhat.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id t10si3220936pfk.153.2018.02.14.09.10.07; Wed, 14 Feb 2018 09:10:24 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=redhat.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1033317AbeBNRIv (ORCPT + 99 others); Wed, 14 Feb 2018 12:08:51 -0500 Received: from mail-qt0-f196.google.com ([209.85.216.196]:47044 "EHLO mail-qt0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1033141AbeBNRIu (ORCPT ); Wed, 14 Feb 2018 12:08:50 -0500 Received: by mail-qt0-f196.google.com with SMTP id u6so8546574qtg.13 for ; Wed, 14 Feb 2018 09:08:49 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=Bnp1+jyOuK7EGJG8HfpD7bdXpiKT7WdGkHn+DtoBccg=; b=qYx74VF/12E4W8AS7awLuUDC3zxPUkiw64CiUQKaZ1raZzFZ28pxGJLkQDKzyGA0Ja FAxW2TowYlPwI9tFJf9g6LEowT3oNfYHpJBU3QEDSTBAdOzxHFj0+CCHOmTpWFcJ+1dr +L70xc7j0lDvMEcfpOpaig/NqJIHduRf1EeOph+6F0IbsqcINLyrBp5xxElWRAjhmg8b Ly8bWmpgHqu2EmAlj/lkQCbHf8gd2TjxbEOUaN+WIlQJsvvdxM3UUn57RfjAKPLwRg/z cFx6AcUXCMWob08IW7dZptvyD6HF3iS+Xg9Jd1hjJ3AKRA9n3bVpffa9LUjdFuKvl9mX 2H1g== X-Gm-Message-State: APf1xPAqtInLzUjqUfOW3x0qDKIdbhEH9jJsmVls3IQBiLcz2+/xxYZ2 +Y1MywJse75h+b/ZaPEjwVXJ5hFWNXiREr5VXtQjgA== X-Received: by 10.200.15.250 with SMTP id f55mr8851648qtk.171.1518628129252; Wed, 14 Feb 2018 09:08:49 -0800 (PST) MIME-Version: 1.0 Received: by 10.12.146.213 with HTTP; Wed, 14 Feb 2018 09:08:48 -0800 (PST) In-Reply-To: <20180214185818-mutt-send-email-mst@kernel.org> References: <20180214141850.4017-1-marcandre.lureau@redhat.com> <20180214141850.4017-10-marcandre.lureau@redhat.com> <20180214182714-mutt-send-email-mst@kernel.org> <20180214185818-mutt-send-email-mst@kernel.org> From: Marc-Andre Lureau Date: Wed, 14 Feb 2018 18:08:48 +0100 Message-ID: Subject: Re: [PATCH v14 9/9] RFC: fw_cfg: do DMA read operation To: "Michael S. Tsirkin" Cc: =?UTF-8?B?TWFyYy1BbmRyw6kgTHVyZWF1?= , Linux Kernel Mailing List , Baoquan He , Sergio Lopez Pascual , "Somlo, Gabriel" , xiaolong.ye@intel.com 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 Hi On Wed, Feb 14, 2018 at 5:59 PM, Michael S. Tsirkin wrote: > On Wed, Feb 14, 2018 at 05:52:10PM +0100, Marc-Andre Lureau wrote: >> >> @@ -282,8 +320,9 @@ static int fw_cfg_do_platform_probe(struct platform_device *pdev) >> >> #endif >> >> >> >> /* verify fw_cfg device signature */ >> >> - fw_cfg_read_blob(FW_CFG_SIGNATURE, sig, 0, FW_CFG_SIG_SIZE); >> >> - if (memcmp(sig, "QEMU", FW_CFG_SIG_SIZE) != 0) { >> >> + if (fw_cfg_read_blob(FW_CFG_SIGNATURE, sig, >> >> + 0, FW_CFG_SIG_SIZE, false) < 0 || >> >> + memcmp(sig, "QEMU", FW_CFG_SIG_SIZE) != 0) { >> >> fw_cfg_io_cleanup(); >> >> return -ENODEV; >> >> } >> > >> > Rather than add dead code, how about a promise not to >> > fail if dma is disabled? Patch will be smaller then. >> >> Even with dma disabled, you could have a locking bug which was >> silently ignored before and is now taken into account. > > I see. I'd start with a patch reporting errors to users then. > That would be a bugfix and can be merged for this version. silently is the wrong word for it, there is already a WARN(). However, the memcmp was done on uninitialzed sig[] (which likely resulted in returning -ENODEV in the function) Now it can check if the function failed to read before doing the memcmp. Hope that clarifies it. > >> > >> >> @@ -466,8 +505,8 @@ static ssize_t fw_cfg_sysfs_read_raw(struct file *filp, struct kobject *kobj, >> >> if (count > entry->size - pos) >> >> count = entry->size - pos; >> >> >> >> - fw_cfg_read_blob(entry->select, buf, pos, count); >> >> - return count; >> >> + /* do not use DMA, virt_to_phys(buf) might not be ok */ >> >> + return fw_cfg_read_blob(entry->select, buf, pos, count, false); >> >> } >> >> >> >> static struct bin_attribute fw_cfg_sysfs_attr_raw = { >> >> @@ -632,7 +671,12 @@ static int fw_cfg_register_dir_entries(void) >> >> struct fw_cfg_file *dir; >> >> size_t dir_size; >> >> >> >> - fw_cfg_read_blob(FW_CFG_FILE_DIR, &files.count, 0, sizeof(files.count)); >> >> + ret = fw_cfg_read_blob(FW_CFG_FILE_DIR, &files.count, >> >> + 0, sizeof(files.count), false); >> >> + if (ret < 0) { >> >> + return ret; >> >> + } >> >> + >> >> count = be32_to_cpu(files.count); >> >> dir_size = count * sizeof(struct fw_cfg_file); >> >> >> >> @@ -640,7 +684,11 @@ static int fw_cfg_register_dir_entries(void) >> >> if (!dir) >> >> return -ENOMEM; >> >> >> >> - fw_cfg_read_blob(FW_CFG_FILE_DIR, dir, sizeof(files.count), dir_size); >> >> + ret = fw_cfg_read_blob(FW_CFG_FILE_DIR, dir, >> >> + sizeof(files.count), dir_size, false); >> >> + if (ret < 0) { >> >> + goto end; >> >> + } >> >> >> >> for (i = 0; i < count; i++) { >> >> ret = fw_cfg_register_file(&dir[i]); >> >> @@ -648,6 +696,7 @@ static int fw_cfg_register_dir_entries(void) >> >> break; >> >> } >> >> >> >> +end: >> >> kfree(dir); >> >> return ret; >> >> } >> >> @@ -688,7 +737,10 @@ static int fw_cfg_sysfs_probe(struct platform_device *pdev) >> >> goto err_probe; >> >> >> >> /* get revision number, add matching top-level attribute */ >> >> - fw_cfg_read_blob(FW_CFG_ID, &rev, 0, sizeof(rev)); >> >> + err = fw_cfg_read_blob(FW_CFG_ID, &rev, 0, sizeof(rev), false); >> >> + if (err < 0) { >> >> + goto err_probe; >> >> + } >> >> fw_cfg_rev = le32_to_cpu(rev); >> >> err = sysfs_create_file(fw_cfg_top_ko, &fw_cfg_rev_attr.attr); >> >> if (err) >> >> -- >> >> 2.16.1.73.g5832b7e9f2