[bug report] iommu/arm-smmu-v3: Refactor write_ctx_desc

[bug report] iommu/arm-smmu-v3: Refactor write_ctx_desc

[Dan Carpenter]

Hello Michael Shavit,

The patch 24503148c545: "iommu/arm-smmu-v3: Refactor write_ctx_desc"
from Sep 15, 2023 (linux-next), leads to the following Smatch static
checker warning:

	drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c:1036 arm_smmu_get_cd_ptr()
	warn: sleeping in atomic context

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
    1022 static __le64 *arm_smmu_get_cd_ptr(struct arm_smmu_master *master, u32 ssid)
    1023 {
    1024         __le64 *l1ptr;
    1025         unsigned int idx;
    1026         struct arm_smmu_l1_ctx_desc *l1_desc;
    1027         struct arm_smmu_device *smmu = master->smmu;
    1028         struct arm_smmu_ctx_desc_cfg *cd_table = &master->cd_table;
    1029 
    1030         if (cd_table->s1fmt == STRTAB_STE_0_S1FMT_LINEAR)
    1031                 return cd_table->cdtab + ssid * CTXDESC_CD_DWORDS;
    1032 
    1033         idx = ssid >> CTXDESC_SPLIT;
    1034         l1_desc = &cd_table->l1_desc[idx];
    1035         if (!l1_desc->l2ptr) {
                     ^^^^^^^^^^^^^^^
I don't know if l1_desc->l2ptr can be NULL for the call trees in
question so this might be a false positive.  You'd need to enable
CONFIG_DEBUG_ATOMIC_SLEEP=y to see these warnings at runtime.


--> 1036                 if (arm_smmu_alloc_cd_leaf_table(smmu, l1_desc))
                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Sleeping allocation.

    1037                         return NULL;
    1038 
    1039                 l1ptr = cd_table->cdtab + idx * CTXDESC_L1_DESC_DWORDS;
    1040                 arm_smmu_write_cd_l1_desc(l1ptr, l1_desc);
    1041                 /* An invalid L1CD can be cached */
    1042                 arm_smmu_sync_cd(master, ssid, false);
    1043         }
    1044         idx = ssid & (CTXDESC_L2_ENTRIES - 1);
    1045         return l1_desc->l2ptr + idx * CTXDESC_CD_DWORDS;
    1046 }

The call trees which trigger this warning are:

arm_smmu_update_ctx_desc_devices() <- disables preempt
arm_smmu_mmu_notifier_get() <- disables preempt
-> arm_smmu_write_ctx_desc()
   -> arm_smmu_get_cd_ptr()

The patch adds new spinlocks to disable preemption.

regards,
dan carpenter

Re: [bug report] iommu/arm-smmu-v3: Refactor write_ctx_desc

[Michael Shavit]

On Thu, Feb 1, 2024 at 9:18 PM Dan Carpenter <dan.carpenter@linaro.org> wrote:
>
> Hello Michael Shavit,
>
> The patch 24503148c545: "iommu/arm-smmu-v3: Refactor write_ctx_desc"
> from Sep 15, 2023 (linux-next), leads to the following Smatch static
> checker warning:
>
>         drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c:1036 arm_smmu_get_cd_ptr()
>         warn: sleeping in atomic context
>
> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>     1022 static __le64 *arm_smmu_get_cd_ptr(struct arm_smmu_master *master, u32 ssid)
>     1023 {
>     1024         __le64 *l1ptr;
>     1025         unsigned int idx;
>     1026         struct arm_smmu_l1_ctx_desc *l1_desc;
>     1027         struct arm_smmu_device *smmu = master->smmu;
>     1028         struct arm_smmu_ctx_desc_cfg *cd_table = &master->cd_table;
>     1029
>     1030         if (cd_table->s1fmt == STRTAB_STE_0_S1FMT_LINEAR)
>     1031                 return cd_table->cdtab + ssid * CTXDESC_CD_DWORDS;
>     1032
>     1033         idx = ssid >> CTXDESC_SPLIT;
>     1034         l1_desc = &cd_table->l1_desc[idx];
>     1035         if (!l1_desc->l2ptr) {
>                      ^^^^^^^^^^^^^^^
> I don't know if l1_desc->l2ptr can be NULL for the call trees in
> question so this might be a false positive.  You'd need to enable
> CONFIG_DEBUG_ATOMIC_SLEEP=y to see these warnings at runtime.
>
>
> --> 1036                 if (arm_smmu_alloc_cd_leaf_table(smmu, l1_desc))
>                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Sleeping allocation.
>
>     1037                         return NULL;
>     1038
>     1039                 l1ptr = cd_table->cdtab + idx * CTXDESC_L1_DESC_DWORDS;
>     1040                 arm_smmu_write_cd_l1_desc(l1ptr, l1_desc);
>     1041                 /* An invalid L1CD can be cached */
>     1042                 arm_smmu_sync_cd(master, ssid, false);
>     1043         }
>     1044         idx = ssid & (CTXDESC_L2_ENTRIES - 1);
>     1045         return l1_desc->l2ptr + idx * CTXDESC_CD_DWORDS;
>     1046 }
>
> The call trees which trigger this warning are:
>
> arm_smmu_update_ctx_desc_devices() <- disables preempt
This call is a false positive since arm_smmu_update_ctx_desc_devices
only ever updates existing CD entries, which doesn't require any
allocation.

> arm_smmu_mmu_notifier_get() <- disables preempt
> -> arm_smmu_write_ctx_desc()
>    -> arm_smmu_get_cd_ptr()

Thanks for the report Dan. This call tree looks like a true positive
if SVA is used and s1fmt is set to STRTAB_STE_0_S1FMT_64K_L2.

Jason's "iommu/arm-smmu-v3: Lift CD programming out of the SVA
notifier code" patch would resolve this by moving the
arm_smmu_get_cd_ptr() call that causes the allocation further up the
call tree (although the intent was to prevent handling of failing
allocations).

Jason, should we consider moving arm_smmu_get_cd_ptr() further up the
call-stack as a standalone patch to remediate this report in the
shorter term? It won't make this static checker happy, but should at
least resolve any true positives.

(P.S: on vacation 01/26-02/18, scanning and replying to emails on a
best-effort basis so apologies for any delays)

Michael Shavit

Re: [bug report] iommu/arm-smmu-v3: Refactor write_ctx_desc

[Jason Gunthorpe]

On Fri, Feb 02, 2024 at 09:33:14PM +0900, Michael Shavit wrote:
> Jason, should we consider moving arm_smmu_get_cd_ptr() further up the
> call-stack as a standalone patch to remediate this report in the
> shorter term? It won't make this static checker happy, but should at
> least resolve any true positives.

Moving is the right answer, but I suspect it is too hard for rc. Lets
leave it to my series that already exists.

Instead we can preallocate:

--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
@@ -588,6 +588,11 @@ static int arm_smmu_sva_set_dev_pasid(struct iommu_domain *domain,
 {
        int ret = 0;
        struct mm_struct *mm = domain->mm;
+       struct arm_smmu_master *master = dev_iommu_priv_get(dev);
+
+       /* Preallocate the required leafs outside locks */
+       if (!arm_smmu_get_cd_ptr(master, id))
+               return -ENOMEM;
 
        mutex_lock(&sva_lock);
        ret = __arm_smmu_sva_bind(dev, mm);

It won't silence Dan's warning warning but it will correct the bug for
now.

I'll send a patch

Jason

Re: [bug report] iommu/arm-smmu-v3: Refactor write_ctx_desc

[Dan Carpenter]

On Fri, Feb 02, 2024 at 09:35:57AM -0400, Jason Gunthorpe wrote:
> 
> It won't silence Dan's warning warning but it will correct the bug for
> now.
> 

I could just modify the database to say that
arm_smmu_update_ctx_desc_devices() doesn't call arm_smmu_write_ctx_desc()
with preempt disabled.  I don't think it will silence any true
positives.  That's a one liner for me.

regards,
dan carpenter

Re: [bug report] iommu/arm-smmu-v3: Refactor write_ctx_desc

[Jason Gunthorpe]

On Fri, Feb 02, 2024 at 05:45:43PM +0300, Dan Carpenter wrote:
> On Fri, Feb 02, 2024 at 09:35:57AM -0400, Jason Gunthorpe wrote:
> > It won't silence Dan's warning warning but it will correct the bug for
> > now.
> 
> I could just modify the database to say that
> arm_smmu_update_ctx_desc_devices() doesn't call arm_smmu_write_ctx_desc()
> with preempt disabled.  I don't think it will silence any true
> positives.  That's a one liner for me.

But it does and it found this bug :)

After my series we have things like this:

		cdptr = arm_smmu_get_cd_ptr(master, master_domain->ssid);
		if (WARN_ON(!cdptr))
			continue;

Which indicates the paths that expect to have been pre-allocated. We
could also have a no_alloc parameter to really make the checker happy?

Jason

Re: [bug report] iommu/arm-smmu-v3: Refactor write_ctx_desc

[Dan Carpenter]

On Fri, Feb 02, 2024 at 10:59:07AM -0400, Jason Gunthorpe wrote:
> On Fri, Feb 02, 2024 at 05:45:43PM +0300, Dan Carpenter wrote:
> > On Fri, Feb 02, 2024 at 09:35:57AM -0400, Jason Gunthorpe wrote:
> > > It won't silence Dan's warning warning but it will correct the bug for
> > > now.
> > 
> > I could just modify the database to say that
> > arm_smmu_update_ctx_desc_devices() doesn't call arm_smmu_write_ctx_desc()
> > with preempt disabled.  I don't think it will silence any true
> > positives.  That's a one liner for me.
> 
> But it does and it found this bug :)
> 

No, it would just remove the one link between arm_smmu_update_ctx_desc_devices()
and arm_smmu_write_ctx_desc().  The other path would still warn.

Actually, I think even after you patch there is still a warning...  To
be honest, I don't normally stress about false positivives.  I report
all new warnings and everything that doesn't get fixed is a false
positive.  If this false posotive becomes an issue then I can spend time
on it at that point.

> After my series we have things like this:
> 
> 		cdptr = arm_smmu_get_cd_ptr(master, master_domain->ssid);
> 		if (WARN_ON(!cdptr))
> 			continue;
> 
> Which indicates the paths that expect to have been pre-allocated. We
> could also have a no_alloc parameter to really make the checker happy?

I don't think so, although I could write a custom function to handle
that parameter.

You're talking about relationships between variables.  If a is foo that
means b is bar.  And there are tons and tons of possible relationships
and it's difficult to know which relationships matter.  Inside a
function Smatch tries to track everything and then at the function
boundary almost all the relationship information is lost.

regards,
dan carpenter

Re: [bug report] iommu/arm-smmu-v3: Refactor write_ctx_desc

[Michael Shavit]

On Fri, Feb 2, 2024 at 10:36 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
>
> On Fri, Feb 02, 2024 at 09:33:14PM +0900, Michael Shavit wrote:
> > Jason, should we consider moving arm_smmu_get_cd_ptr() further up the
> > call-stack as a standalone patch to remediate this report in the
> > shorter term? It won't make this static checker happy, but should at
> > least resolve any true positives.
>
> Moving is the right answer, but I suspect it is too hard for rc. Lets
> leave it to my series that already exists.
>
> Instead we can preallocate:

Yeah that's what I meant; thanks for the help with the fix!

>
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
> @@ -588,6 +588,11 @@ static int arm_smmu_sva_set_dev_pasid(struct iommu_domain *domain,
>  {
>         int ret = 0;
>         struct mm_struct *mm = domain->mm;
> +       struct arm_smmu_master *master = dev_iommu_priv_get(dev);
> +
> +       /* Preallocate the required leafs outside locks */
> +       if (!arm_smmu_get_cd_ptr(master, id))
> +               return -ENOMEM;
>
>         mutex_lock(&sva_lock);
>         ret = __arm_smmu_sva_bind(dev, mm);
>
> It won't silence Dan's warning warning but it will correct the bug for
> now.
>
> I'll send a patch
>
> Jason