Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 9 Jun 2023 16:18:08 +0300
From:   Dan Carpenter <dan.carpenter@linaro.org>
To:     Sai Krishna <saikrishnag@marvell.com>
Cc:     davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
        pabeni@redhat.com, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org, sgoutham@marvell.com,
        maciej.fijalkowski@intel.com,
        Naveen Mamindlapalli <naveenm@marvell.com>
Subject: Re: [net PATCH v2] octeontx2-af: Move validation of ptp pointer
 before its usage
Message-ID: <880d628e-18bf-44a1-a55f-ffbe1777dd2b@kadam.mountain>
References: <20230609115806.2625564-1-saikrishnag@marvell.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230609115806.2625564-1-saikrishnag@marvell.com>
Precedence: bulk

On Fri, Jun 09, 2023 at 05:28:06PM +0530, Sai Krishna wrote:
> @@ -428,7 +427,7 @@ static int ptp_probe(struct pci_dev *pdev,
>  	return 0;
>  
>  error_free:
> -	devm_kfree(dev, ptp);
> +	kfree(ptp);

Yeah.  It's strange any time we call devm_kfree()...  So there is
something here which I have not understood.

>  
>  error:
>  	/* For `ptp_get()` we need to differentiate between the case

This probe function is super weird how it returns success on the failure
path.  One concern, I had initially was that if anything returns
-EPROBE_DEFER then we cannot recover.  That's not possible in the
current code, but it makes me itch...  But here is a different crash.

drivers/net/ethernet/marvell/octeontx2/af/ptp.c
   432  error:
   433          /* For `ptp_get()` we need to differentiate between the case
   434           * when the core has not tried to probe this device and the case when
   435           * the probe failed.  In the later case we pretend that the
   436           * initialization was successful and keep the error in
   437           * `dev->driver_data`.
   438           */
   439          pci_set_drvdata(pdev, ERR_PTR(err));
   440          if (!first_ptp_block)
   441                  first_ptp_block = ERR_PTR(err);

first_ptp_block is NULL for unprobed, an error pointer for probe
failure, or valid pointer.

   442  
   443          return 0;
   444  }

drivers/net/ethernet/marvell/octeontx2/af/ptp.c
   201  struct ptp *ptp_get(void)
   202  {
   203          struct ptp *ptp = first_ptp_block;
                            ^^^^^^^^^^^^^^^^^^^^^^

   204  
   205          /* Check PTP block is present in hardware */
   206          if (!pci_dev_present(ptp_id_table))
   207                  return ERR_PTR(-ENODEV);
   208          /* Check driver is bound to PTP block */
   209          if (!ptp)
   210                  ptp = ERR_PTR(-EPROBE_DEFER);
   211          else
   212                  pci_dev_get(ptp->pdev);
                                    ^^^^^^^^^
if first_ptp_block is an error pointer this will Oops.

   213  
   214          return ptp;
   215  }

regards,
dan carpenter