linux-kernel - Re: [PATCH 4/5] iommu: Regulate errno in ->attach

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 13 Sep 2022 13:14:24 -0700
From:   Nicolin Chen <nicolinc@...dia.com>
To:     Jean-Philippe Brucker <jean-philippe@...aro.org>
CC:     <joro@...tes.org>, <suravee.suthikulpanit@....com>,
        <will@...nel.org>, <robin.murphy@....com>, <robdclark@...il.com>,
        <dwmw2@...radead.org>, <baolu.lu@...ux.intel.com>,
        <agross@...nel.org>, <bjorn.andersson@...aro.org>,
        <konrad.dybcio@...ainline.org>, <matthias.bgg@...il.com>,
        <heiko@...ech.de>, <orsonzhai@...il.com>,
        <baolin.wang@...ux.alibaba.com>, <zhang.lyra@...il.com>,
        <thierry.reding@...il.com>, <jgg@...dia.com>,
        <sricharan@...eaurora.org>, <yong.wu@...iatek.com>,
        <vdumpa@...dia.com>, <jonathanh@...dia.com>, <tglx@...utronix.de>,
        <shameerali.kolothum.thodi@...wei.com>,
        <thunder.leizhen@...wei.com>, <christophe.jaillet@...adoo.fr>,
        <yangyingliang@...wei.com>, <jon@...id-run.com>,
        <iommu@...ts.linux.dev>, <linux-kernel@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-arm-msm@...r.kernel.org>,
        <linux-mediatek@...ts.infradead.org>,
        <linux-rockchip@...ts.infradead.org>,
        <linux-tegra@...r.kernel.org>,
        <virtualization@...ts.linux-foundation.org>
Subject: Re: [PATCH 4/5] iommu: Regulate errno in ->attach_dev callback
 functions

Hi Jean,

On Tue, Sep 13, 2022 at 01:27:03PM +0100, Jean-Philippe Brucker wrote:
> External email: Use caution opening links or attachments
> 
> 
> Hi Nicolin,
> 
> On Tue, Sep 13, 2022 at 01:24:47AM -0700, Nicolin Chen wrote:
> > Following the new rules in include/linux/iommu.h kdocs, update all drivers
> > ->attach_dev callback functions to return ENODEV error code for all device
> > specific errors. It particularly excludes EINVAL from being used for such
> > error cases. For the same purpose, also replace one EINVAL with ENOMEM in
> > mtk_iommu driver.
> >
> > Note that the virtio-iommu does a viommu_domain_map_identity() call, which
> > returns either 0 or ENOMEM at this moment. Change to "return ret" directly
> > to allow it to pass an EINVAL in the future.
> [...]
> > diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
> > index 80151176ba12..874c01634d2b 100644
> > --- a/drivers/iommu/virtio-iommu.c
> > +++ b/drivers/iommu/virtio-iommu.c
> > @@ -696,7 +696,7 @@ static int viommu_domain_finalise(struct viommu_endpoint *vdev,
> >               if (ret) {
> >                       ida_free(&viommu->domain_ids, vdomain->id);
> >                       vdomain->viommu = NULL;
> > -                     return -EOPNOTSUPP;
> > +                     return ret;
> 
> I think in the future it will be too easy to forget about the constrained
> return value of attach() while modifying some other part of the driver,
> and let an external helper return EINVAL. So I'd rather not propagate ret
> from outside of viommu_domain_attach() and finalise().
> 
> For the same reason I do prefer this solution over EMEDIUMTYPE, because
> it's too tempting to use exotic errno when they seem appropriate instead
> of boring ENODEV and EINVAL. The alternative would be adding a special
> purpose code to linux/errno.h, similarly to EPROBE_DEFER, but that might
> be excessive.
> 
> Since we can't guarantee that APIs like virtio or ida won't ever return
> EINVAL, we should set all return values:

Thanks for the inputs. Assuming your attached patch isn't officially
sent, I will group it into my next version.

Similarly, I will also double check other drivers, to make sure all
of them have explicit return values, other than "ret".