Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v4 10/10] scsi: ufs: Apply more limitations to user access
To:     Can Guo <cang@codeaurora.org>, asutoshd@codeaurora.org,
        nguyenb@codeaurora.org, hongwus@codeaurora.org,
        ziqichen@codeaurora.org, linux-scsi@vger.kernel.org,
        kernel-team@android.com
Cc:     Alim Akhtar <alim.akhtar@samsung.com>,
        Avri Altman <avri.altman@wdc.com>,
        "James E.J. Bottomley" <jejb@linux.ibm.com>,
        "Martin K. Petersen" <martin.petersen@oracle.com>,
        Adrian Hunter <adrian.hunter@intel.com>,
        Bean Huo <beanhuo@micron.com>,
        Stanley Chu <stanley.chu@mediatek.com>,
        Keoseong Park <keosung.park@samsung.com>,
        "Gustavo A. R. Silva" <gustavoars@kernel.org>,
        Jaegeuk Kim <jaegeuk@kernel.org>,
        Kiwoong Kim <kwmad.kim@samsung.com>,
        Satya Tangirala <satyat@google.com>,
        open list <linux-kernel@vger.kernel.org>
References: <1624433711-9339-1-git-send-email-cang@codeaurora.org>
 <1624433711-9339-12-git-send-email-cang@codeaurora.org>
From:   Bart Van Assche <bvanassche@acm.org>
Message-ID: <89a3c8bf-bbfc-4a2a-73f0-a0db956fbf0e@acm.org>
Date:   Wed, 23 Jun 2021 14:51:25 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.11.0
MIME-Version: 1.0
In-Reply-To: <1624433711-9339-12-git-send-email-cang@codeaurora.org>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 6/23/21 12:35 AM, Can Guo wrote:
> +int ufshcd_get_user_access(struct ufs_hba *hba)
> +__acquires(&hba->host_sem)
> +{
> +	down(&hba->host_sem);
> +	if (!ufshcd_is_user_access_allowed(hba)) {
> +		up(&hba->host_sem);
> +		return -EBUSY;
> +	}
> +	if (ufshcd_rpm_get_sync(hba)) {
> +		ufshcd_rpm_put_sync(hba);
> +		up(&hba->host_sem);
> +		return -EBUSY;
> +	}
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(ufshcd_get_user_access);
> +
> +void ufshcd_put_user_access(struct ufs_hba *hba)
> +__releases(&hba->host_sem)
> +{
> +	ufshcd_rpm_put_sync(hba);
> +	up(&hba->host_sem);
> +}
> +EXPORT_SYMBOL_GPL(ufshcd_put_user_access);

Please indent __acquires() and __releases() annotations by one tab as is
done elsewhere in the kernel.

>  static inline bool ufshcd_is_user_access_allowed(struct ufs_hba *hba)
>  {
> -	return !hba->shutting_down;
> +	return !hba->shutting_down && !hba->is_sys_suspended &&
> +		!hba->is_wlu_sys_suspended &&
> +		hba->ufshcd_state == UFSHCD_STATE_OPERATIONAL;
>  }

Is my understanding of the following correct?
- ufshcd_is_user_access_allowed() is not in the hot path and hence
  should not be inline.
- The hba->shutting_down member variable is set from inside a shutdown
  callback. Hence, the hba->shutting_down test can be left out since
  no UFS sysfs attributes are accessed after shutdown has started.
- During system suspend, user space software is paused before the device
  driver freeze callbacks are invoked. Hence, the hba->is_sys_suspended
  check can be left out.
- If a LUN is runtime suspended, it should be resumed if accessed from
  user space instead of failing user space accesses. In other words, the
  hba->is_wlu_sys_suspended check seems inappropriate to me.
- If the HBA is not in an operational state, user space accesses
  should be blocked until error handling has finished. After error
  handling has finished, the user space access should fail if and only
  if error handling failed.

Thanks,

Bart.