Re: [PATCH v4 05/11] iommu/sva: Assign a PASID to mm on PASID allocation and free it on mm exit

From: zhangfei.gao@xxxxxxxxxxx
Date: Thu Apr 14 2022 - 06:08:35 EST



On 2022/4/12 下午11:35, zhangfei.gao@xxxxxxxxxxx wrote:
Hi, Fenghua

On 2022/4/12 下午9:41, Fenghua Yu wrote:
Hi, Zhangfei,

On Tue, Apr 12, 2022 at 03:04:09PM +0800, zhangfei.gao@xxxxxxxxxxx wrote:

On 2022/4/11 下午10:52, Dave Hansen wrote:
On 4/11/22 07:44, zhangfei.gao@xxxxxxxxxxx wrote:
On 2022/4/11 下午10:36, Dave Hansen wrote:
On 4/11/22 07:20, zhangfei.gao@xxxxxxxxxxx wrote:
Is there nothing before this call trace?  Usually there will be at least
some warning text.
I added dump_stack() in ioasid_free.
Hold on a sec, though...

What's the *problem* here?  Did something break or are you just saying
that something looks weird to _you_?
After this, nginx is not working at all, and hardware reports error.
Suppose the the master use the ioasid for init, but got freed.

hardware reports:
[  152.731869] hisi_sec2 0000:76:00.0: qm_acc_do_task_timeout [error status=0x20] found
[  152.739657] hisi_sec2 0000:76:00.0: qm_acc_wb_not_ready_timeout [error status=0x40] found
[  152.747877] hisi_sec2 0000:76:00.0: sec_fsm_hbeat_rint [error status=0x20] found
[  152.755340] hisi_sec2 0000:76:00.0: Controller resetting...
[  152.762044] hisi_sec2 0000:76:00.0: QM mailbox operation timeout!
[  152.768198] hisi_sec2 0000:76:00.0: Failed to dump sqc!
[  152.773490] hisi_sec2 0000:76:00.0: Failed to drain out data for stopping!
[  152.781426] hisi_sec2 0000:76:00.0: QM mailbox is busy to start!
[  152.787468] hisi_sec2 0000:76:00.0: Failed to dump sqc!
[  152.792753] hisi_sec2 0000:76:00.0: Failed to drain out data for stopping!
[  152.800685] hisi_sec2 0000:76:00.0: QM mailbox is busy to start!
[  152.806730] hisi_sec2 0000:76:00.0: Failed to dump sqc!
[  152.812017] hisi_sec2 0000:76:00.0: Failed to drain out data for stopping!
[  152.819946] hisi_sec2 0000:76:00.0: QM mailbox is busy to start!
[  152.825992] hisi_sec2 0000:76:00.0: Failed to dump sqc!
That would have been awfully handy information to have in an initial bug report. :)
Is there a chance you could dump out that ioasid alloc *and* free information in ioasid_alloc/free()?  This could be some kind of problem with the allocator, or with copying the ioasid at fork.
The issue is nginx master process init resource, start daemon process, then
master process quit and free ioasid.
The daemon nginx process is not the original master process.

master process:  init resource
driver -> iommu_sva_bind_device -> ioasid_alloc
Which code in the master process/daemon calls driver->iommu_sva_unbind_device?
Our calling sequence is nginx -> openssl -> openssl engine ->  kernel driver
The calling entrence should be ngx_ssl_init : OPENSSL_config(NULL);

nginx:
src/event/ngx_event_openssl.c
ngx_ssl_init
if (OPENSSL_init_ssl(OPENSSL_INIT_LOAD_CONFIG, NULL) == 0)

I add some print.

/usr/local/nginx$ sudo sbin/nginx
ngx_ssl_init pid=2361
bind_fn
ngx_openssl_create_conf pid=2361
hisi sec init Kunpeng920!
ngx_ssl_create pid=2361
ngx_ssl_certificates pid=2361
ngx_ssl_certificate pid=2361
uadk_e_wd_digest_init
hisi sec init Kunpeng920!
ngx_ssl_ciphers pid=2361
ngx_daemon pid=2361 fork daemon
master pid=2361 will exit                                // here master process is exit
fork return 0 pid=2364                                       // here daemon process started
ngx_daemon fork ngx_pid=2364, ngx_parent=2361
$ ps -aux | grep nginx
root        2364  0.0  0.0  31324 15380 ?        Ssl  15:21   0:00 nginx: master process sbin/nginx
nobody      2366  0.0  0.0  32304 16448 ?        Sl   15:21   0:00 nginx: worker process
linaro      2371  0.0  0.0   7696  2048 pts/0    S+   15:22   0:00 grep --color=auto nginx

nginx
src/os/unix/ngx_daemon.c
ngx_daemon(ngx_log_t *log)
{
    int  fd;

    switch (fork()) {
    case -1:
        ngx_log_error(NGX_LOG_EMERG, log, ngx_errno, "fork() failed");
        return NGX_ERROR;

    case 0:
       // here fork daemon process
        break;

    default:
      // master process directly exit, and release mm as well as ioasid
        exit(0);
    }

     // only daemon process
    ngx_parent = ngx_pid;
    ngx_pid = ngx_getpid();
Any suggestion?
ioasid is per process or per mm. A daemon process shouldn't share the same
ioasid with any other process with even its parent process. Its parent gets
an ioasid and frees it on exit. The ioasid is gone and shouldn't be used
by its child process.

Each daemon process should call driver -> iommu_sva_bind_device -> ioasid_alloc
to get its own ioasid/PASID. On daemon quit, the ioasid is freed.

That means nqnix needs to be changed.

Agree with Dave, I think user space should not be broken.

Thanks

Any plan about this regression?
Currently I need this patch to workaround the issue.

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
index 22ddd05bbdcd..2d74ac53d11c 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
@@ -4,6 +4,7 @@
  */

 #include <linux/mm.h>
+#include <linux/sched/mm.h>
 #include <linux/mmu_context.h>
 #include <linux/mmu_notifier.h>
 #include <linux/slab.h>
@@ -363,6 +364,7 @@ arm_smmu_sva_bind(struct device *dev, struct mm_struct *mm, void *drvdata)

        mutex_lock(&sva_lock);
        handle = __arm_smmu_sva_bind(dev, mm);
+       mmget(mm);
        mutex_unlock(&sva_lock);
        return handle;
 }
@@ -377,6 +379,7 @@ void arm_smmu_sva_unbind(struct iommu_sva *handle)
                arm_smmu_mmu_notifier_put(bond->smmu_mn);
                kfree(bond);
        }
+       mmput(bond->mm);
        mutex_unlock(&sva_lock);
 }

Thanks