Re: [dpdk-dev] [PATCH v2] crypto/caam_jr: fix wrong check of fd

[Gagandeep Singh <G.Singh@nxp.com>]

> -----Original Message-----
> From: wangyunjian <wangyunjian@huawei.com>
> Sent: Monday, May 11, 2020 10:07 AM
> To: dev@dpdk.org
> Cc: Gagandeep Singh <G.Singh@nxp.com>; Hemant Agrawal
> <hemant.agrawal@nxp.com>; jerry.lilijun@huawei.com;
> xudingke@huawei.com; Yunjian Wang <wangyunjian@huawei.com>;
> stable@dpdk.org
> Subject: [dpdk-dev] [PATCH v2] crypto/caam_jr: fix wrong check of fd
> 
> From: Yunjian Wang <wangyunjian@huawei.com>
> 
> Zero is a valid fd. It will fail to check the fd if the fd is zero.
> The "job_ring->uio_fd" is an fd, so define it as "int".
> 
> Fixes: e7a45f3cc245 ("crypto/caam_jr: add UIO specific operations")
> Fixes: a5e1018d5e67 ("crypto/caam_jr: add routines to configure HW")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
> ---
> v2:
>  * Change "job_ring->uio_fd" type suggested by Gagandeep Singh
> ---
>  drivers/crypto/caam_jr/caam_jr.c             |  6 ++++--
>  drivers/crypto/caam_jr/caam_jr_hw_specific.h |  2 +-
>  drivers/crypto/caam_jr/caam_jr_pvt.h         |  5 +++--
>  drivers/crypto/caam_jr/caam_jr_uio.c         | 21 ++++++++++++++------
>  4 files changed, 23 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/crypto/caam_jr/caam_jr.c
> b/drivers/crypto/caam_jr/caam_jr.c
> index 5a29dd169..f62d46546 100644
> --- a/drivers/crypto/caam_jr/caam_jr.c
> +++ b/drivers/crypto/caam_jr/caam_jr.c
> @@ -2074,7 +2074,7 @@ static struct rte_security_ops caam_jr_security_ops =
> {
>  static void
>  close_job_ring(struct sec_job_ring_t *job_ring)
>  {
> -	if (job_ring->irq_fd) {
> +	if (job_ring->irq_fd != -1) {
>  		/* Producer index is frozen. If consumer index is not equal
>  		 * with producer index, then we have descs to flush.
>  		 */
> @@ -2187,7 +2187,7 @@ caam_jr_dev_uninit(struct rte_cryptodev *dev)
>   *
>   */
>  static void *
> -init_job_ring(void *reg_base_addr, uint32_t irq_id)
> +init_job_ring(void *reg_base_addr, int irq_id)
>  {
>  	struct sec_job_ring_t *job_ring = NULL;
>  	int i, ret = 0;
> @@ -2466,6 +2466,8 @@
> RTE_PMD_REGISTER_CRYPTO_DRIVER(caam_jr_crypto_drv,
> cryptodev_caam_jr_drv.driver,
> 
>  RTE_INIT(caam_jr_init_log)
>  {
> +	sec_init();
> +
Why are you calling a fd initialization function in log registration constructor?
This is not the right place for any initialization other than logs.
Also,  sec_init function will not work here, as you are using "g_uio_jr_num" whose value will always be 0 during this function call.

>  	caam_jr_logtype = rte_log_register("pmd.crypto.caam");
>  	if (caam_jr_logtype >= 0)
>  		rte_log_set_level(caam_jr_logtype, RTE_LOG_NOTICE);
> diff --git a/drivers/crypto/caam_jr/caam_jr_hw_specific.h
> b/drivers/crypto/caam_jr/caam_jr_hw_specific.h
> index 5f58a585d..ec4539d1b 100644
> --- a/drivers/crypto/caam_jr/caam_jr_hw_specific.h
> +++ b/drivers/crypto/caam_jr/caam_jr_hw_specific.h
> @@ -360,7 +360,7 @@ struct sec_job_ring_t {
>  				 * bitwise operations.
>  				 */
> 
> -	uint32_t irq_fd;	/* The file descriptor used for polling from
> +	int irq_fd;	/* The file descriptor used for polling from
>  				 * user space for interrupts notifications
>  				 */
>  	uint32_t jr_mode;	/* Model used by SEC Driver to receive
> diff --git a/drivers/crypto/caam_jr/caam_jr_pvt.h
> b/drivers/crypto/caam_jr/caam_jr_pvt.h
> index 98cd4438a..fa9e86ea6 100644
> --- a/drivers/crypto/caam_jr/caam_jr_pvt.h
> +++ b/drivers/crypto/caam_jr/caam_jr_pvt.h
> @@ -216,16 +216,17 @@ calc_chksum(void *buffer, int len)
>  }
>  struct uio_job_ring {
>  	uint32_t jr_id;
> -	uint32_t uio_fd;
> +	int uio_fd;
>  	void *register_base_addr;
>  	int map_size;
>  	int uio_minor_number;
>  };
> 
>  int sec_cleanup(void);
> +void sec_init(void);
>  int sec_configure(void);
>  struct uio_job_ring *config_job_ring(void);
> -void free_job_ring(uint32_t uio_fd);
> +void free_job_ring(int uio_fd);
> 
>  /* For Dma memory allocation of specified length and alignment */
>  static inline void *
> diff --git a/drivers/crypto/caam_jr/caam_jr_uio.c
> b/drivers/crypto/caam_jr/caam_jr_uio.c
> index b1bb44ca4..133b32084 100644
> --- a/drivers/crypto/caam_jr/caam_jr_uio.c
> +++ b/drivers/crypto/caam_jr/caam_jr_uio.c
> @@ -145,7 +145,7 @@ file_read_first_line(const char root[], const char
> subdir[],
>  		 "%s/%s/%s", root, subdir, filename);
> 
>  	fd = open(absolute_file_name, O_RDONLY);
> -	SEC_ASSERT(fd > 0, fd, "Error opening file %s",
> +	SEC_ASSERT(fd >= 0, fd, "Error opening file %s",
>  			absolute_file_name);
> 
>  	/* read UIO device name from first line in file */
> @@ -322,7 +322,7 @@ uio_map_registers(int uio_device_fd, int uio_device_id,
>  }
> 
>  void
> -free_job_ring(uint32_t uio_fd)
> +free_job_ring(int uio_fd)
>  {
>  	struct uio_job_ring *job_ring = NULL;
>  	int i;

Please update the check  "if (!uio_fd)".

> @@ -347,7 +347,7 @@ free_job_ring(uint32_t uio_fd)
>  			job_ring->jr_id, job_ring->uio_fd);
>  	close(job_ring->uio_fd);
>  	g_uio_jr_num--;
> -	job_ring->uio_fd = 0;
> +	job_ring->uio_fd = -1;
>  	if (job_ring->register_base_addr == NULL)
>  		return;
> 
> @@ -370,7 +370,7 @@ uio_job_ring *config_job_ring(void)
>  	int i;
> 
>  	for (i = 0; i < MAX_SEC_JOB_RINGS; i++) {
> -		if (g_uio_job_ring[i].uio_fd == 0) {
> +		if (g_uio_job_ring[i].uio_fd == -1) {
>  			job_ring = &g_uio_job_ring[i];
>  			g_uio_jr_num++;
>  			break;
> @@ -389,7 +389,7 @@ uio_job_ring *config_job_ring(void)
> 
>  	/* Open device file */
>  	job_ring->uio_fd = open(uio_device_file_name, O_RDWR);
> -	SEC_ASSERT(job_ring->uio_fd > 0, NULL,
> +	SEC_ASSERT(job_ring->uio_fd >= 0, NULL,
>  		"Failed to open UIO device file for job ring %d",
>  		job_ring->jr_id);
> 
> @@ -488,12 +488,21 @@ sec_cleanup(void)
>  		/* I need to close the fd after shutdown UIO commands need to
> be
>  		 * sent using the fd
>  		 */
> -		if (job_ring->uio_fd != 0) {
> +		if (job_ring->uio_fd != -1) {
>  			CAAM_JR_INFO(
>  			"Closed device file for job ring %d , fd = %d",
>  			job_ring->jr_id, job_ring->uio_fd);
>  			close(job_ring->uio_fd);
> +			job_ring->uio_fd = -1;
>  		}
>  	}
>  	return 0;
>  }
> +
> +void sec_init(void)
> +{
> +	int i;
> +
> +	for (i = 0; i < g_uio_jr_num; i++)
> +		g_uio_job_ring[i].uio_fd = -1;
> +}
> --
> 2.19.1
> 

There are still some functions(sec_uio_send_command, caam_jr_enable_irqs, caam_jr_disable_irqs) in caam_jr_uio.c, which are accepting uio_fd parameter as uint32_t, please update them also.