1. Patchew
  2. linux
  3. View series

[PATCH v2] rbd: replace strcpy() with strscpy()

Siddarth Gundu posted 1 patch 7 months ago 
drivers/block/rbd.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
[PATCH v2] rbd: replace strcpy() with strscpy()
Posted by Siddarth Gundu 7 months ago 
strcpy() is deprecated; use strscpy() instead.

Both the destination and source buffer are of fixed length
so strscpy with 2-arguments is used.

Introduce a typedef for cookie array to improve code clarity.

Link: https://github.com/KSPP/linux/issues/88
Signed-off-by: Siddarth Gundu <siddarthsgml@gmail.com>
---
changes since v1
- added a typedef for cookie arrays

About the typedef: I was a bit hesitant to add it since the kernel
style guide is against adding new typedef but I wanted to follow 
the review feedback for this.

 drivers/block/rbd.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index faafd7ff43d6..863d9c591aa5 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -46,11 +46,14 @@
 #include <linux/slab.h>
 #include <linux/idr.h>
 #include <linux/workqueue.h>
+#include <linux/string.h>
 
 #include "rbd_types.h"
 
 #define RBD_DEBUG	/* Activate rbd_assert() calls */
 
+typedef char rbd_cookie_t[32];
+
 /*
  * Increment the given counter and return its updated value.
  * If the counter is already 0 it will not be incremented.
@@ -411,7 +414,7 @@ struct rbd_device {
 
 	struct rw_semaphore	lock_rwsem;
 	enum rbd_lock_state	lock_state;
-	char			lock_cookie[32];
+	rbd_cookie_t		lock_cookie;
 	struct rbd_client_id	owner_cid;
 	struct work_struct	acquired_lock_work;
 	struct work_struct	released_lock_work;
@@ -3649,12 +3652,12 @@ static void format_lock_cookie(struct rbd_device *rbd_dev, char *buf)
 	mutex_unlock(&rbd_dev->watch_mutex);
 }
 
-static void __rbd_lock(struct rbd_device *rbd_dev, const char *cookie)
+static void __rbd_lock(struct rbd_device *rbd_dev, const rbd_cookie_t cookie)
 {
 	struct rbd_client_id cid = rbd_get_cid(rbd_dev);
 
 	rbd_dev->lock_state = RBD_LOCK_STATE_LOCKED;
-	strcpy(rbd_dev->lock_cookie, cookie);
+	strscpy(rbd_dev->lock_cookie, cookie);
 	rbd_set_owner_cid(rbd_dev, &cid);
 	queue_work(rbd_dev->task_wq, &rbd_dev->acquired_lock_work);
 }
@@ -3665,7 +3668,7 @@ static void __rbd_lock(struct rbd_device *rbd_dev, const char *cookie)
 static int rbd_lock(struct rbd_device *rbd_dev)
 {
 	struct ceph_osd_client *osdc = &rbd_dev->rbd_client->client->osdc;
-	char cookie[32];
+	rbd_cookie_t cookie;
 	int ret;
 
 	WARN_ON(__rbd_is_lock_owner(rbd_dev) ||
@@ -4581,7 +4584,7 @@ static void rbd_unregister_watch(struct rbd_device *rbd_dev)
 static void rbd_reacquire_lock(struct rbd_device *rbd_dev)
 {
 	struct ceph_osd_client *osdc = &rbd_dev->rbd_client->client->osdc;
-	char cookie[32];
+	rbd_cookie_t cookie;
 	int ret;
 
 	if (!rbd_quiesce_lock(rbd_dev))
-- 
2.43.0
Re: [PATCH v2] rbd: replace strcpy() with strscpy()
Posted by Alex Elder 7 months ago 
On 5/19/25 1:38 AM, Siddarth Gundu wrote:
> strcpy() is deprecated; use strscpy() instead.
> 
> Both the destination and source buffer are of fixed length
> so strscpy with 2-arguments is used.
> 
> Introduce a typedef for cookie array to improve code clarity.
> 
> Link: https://github.com/KSPP/linux/issues/88
> Signed-off-by: Siddarth Gundu <siddarthsgml@gmail.com>
> ---
> changes since v1
> - added a typedef for cookie arrays
> 
> About the typedef: I was a bit hesitant to add it since the kernel
> style guide is against adding new typedef but I wanted to follow
> the review feedback for this.

I personally think the typedef here is the appropriate.  But
it's really up to Ilya whether he likes this approach.  Get
his input before you do more.

There's a basic question about whether this is a useful
abstraction.  It's used for "lock cookies" but do they
serve a broader purpose?

The other part of my suggestion was to define functions that
provide an API.  For example:

static inline rbd_cookie_t rbd_cookie_set(rbd_cookie_t cookie, u64 id);
static inline u64 rbd_cookie_get(rbd_cookie_t cookie);

Anyway, before I say any more let's see if Ilya even wants
to go in this direction.  Your original proposal was OK, I
just thought specifying the length might be safer.

					-Alex

>   drivers/block/rbd.c | 13 ++++++++-----
>   1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index faafd7ff43d6..863d9c591aa5 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -46,11 +46,14 @@
>   #include <linux/slab.h>
>   #include <linux/idr.h>
>   #include <linux/workqueue.h>
> +#include <linux/string.h>
>   
>   #include "rbd_types.h"
>   
>   #define RBD_DEBUG	/* Activate rbd_assert() calls */
>   
> +typedef char rbd_cookie_t[32];
> +
>   /*
>    * Increment the given counter and return its updated value.
>    * If the counter is already 0 it will not be incremented.
> @@ -411,7 +414,7 @@ struct rbd_device {
>   
>   	struct rw_semaphore	lock_rwsem;
>   	enum rbd_lock_state	lock_state;
> -	char			lock_cookie[32];
> +	rbd_cookie_t		lock_cookie;
>   	struct rbd_client_id	owner_cid;
>   	struct work_struct	acquired_lock_work;
>   	struct work_struct	released_lock_work;
> @@ -3649,12 +3652,12 @@ static void format_lock_cookie(struct rbd_device *rbd_dev, char *buf)
>   	mutex_unlock(&rbd_dev->watch_mutex);
>   }
>   
> -static void __rbd_lock(struct rbd_device *rbd_dev, const char *cookie)
> +static void __rbd_lock(struct rbd_device *rbd_dev, const rbd_cookie_t cookie)
>   {
>   	struct rbd_client_id cid = rbd_get_cid(rbd_dev);
>   
>   	rbd_dev->lock_state = RBD_LOCK_STATE_LOCKED;
> -	strcpy(rbd_dev->lock_cookie, cookie);
> +	strscpy(rbd_dev->lock_cookie, cookie);
>   	rbd_set_owner_cid(rbd_dev, &cid);
>   	queue_work(rbd_dev->task_wq, &rbd_dev->acquired_lock_work);
>   }
> @@ -3665,7 +3668,7 @@ static void __rbd_lock(struct rbd_device *rbd_dev, const char *cookie)
>   static int rbd_lock(struct rbd_device *rbd_dev)
>   {
>   	struct ceph_osd_client *osdc = &rbd_dev->rbd_client->client->osdc;
> -	char cookie[32];
> +	rbd_cookie_t cookie;
>   	int ret;
>   
>   	WARN_ON(__rbd_is_lock_owner(rbd_dev) ||
> @@ -4581,7 +4584,7 @@ static void rbd_unregister_watch(struct rbd_device *rbd_dev)
>   static void rbd_reacquire_lock(struct rbd_device *rbd_dev)
>   {
>   	struct ceph_osd_client *osdc = &rbd_dev->rbd_client->client->osdc;
> -	char cookie[32];
> +	rbd_cookie_t cookie;
>   	int ret;
>   
>   	if (!rbd_quiesce_lock(rbd_dev))
Re: [PATCH v2] rbd: replace strcpy() with strscpy()
Posted by Siddarth Gundu 7 months ago 
On Tue, May 20, 2025 at 10:14 PM Alex Elder <elder@ieee.org> wrote:

> I personally think the typedef here is the appropriate.  But
> it's really up to Ilya whether he likes this approach.  Get
> his input before you do more.

right, understood.

> There's a basic question about whether this is a useful
> abstraction.  It's used for "lock cookies" but do they
> serve a broader purpose?
>
> The other part of my suggestion was to define functions that
> provide an API.  For example:
>
> static inline rbd_cookie_t rbd_cookie_set(rbd_cookie_t cookie, u64 id);
> static inline u64 rbd_cookie_get(rbd_cookie_t cookie);

I see, I will try implementing such functions. Because of
using typedef I made minimal code changes.
Thanks for the detailed input

> Anyway, before I say any more let's see if Ilya even wants
> to go in this direction.  Your original proposal was OK, I
> just thought specifying the length might be safer.

Alright, I'll wait for feedback before making
any changes.

Thanks for taking time to review the patch

--
With Gratitude
Siddarth Gundu
Re: [PATCH v2] rbd: replace strcpy() with strscpy()
Posted by Jens Axboe 7 months ago 
On 5/20/25 10:44 AM, Alex Elder wrote:
> On 5/19/25 1:38 AM, Siddarth Gundu wrote:
>> strcpy() is deprecated; use strscpy() instead.
>>
>> Both the destination and source buffer are of fixed length
>> so strscpy with 2-arguments is used.
>>
>> Introduce a typedef for cookie array to improve code clarity.
>>
>> Link: https://github.com/KSPP/linux/issues/88
>> Signed-off-by: Siddarth Gundu <siddarthsgml@gmail.com>
>> ---
>> changes since v1
>> - added a typedef for cookie arrays
>>
>> About the typedef: I was a bit hesitant to add it since the kernel
>> style guide is against adding new typedef but I wanted to follow
>> the review feedback for this.
> 
> I personally think the typedef here is the appropriate.  But
> it's really up to Ilya whether he likes this approach.  Get
> his input before you do more.

In any case, this should be 2 patches at that point, not collapsed
into one patch.

-- 
Jens Axboe
Re: [PATCH v2] rbd: replace strcpy() with strscpy()
Posted by Siddarth Gundu 7 months ago 
On Tue, May 20, 2025 at 10:26 PM Jens Axboe <axboe@kernel.dk> wrote:
>
> On 5/20/25 10:44 AM, Alex Elder wrote:
> > On 5/19/25 1:38 AM, Siddarth Gundu wrote:
> >> strcpy() is deprecated; use strscpy() instead.
> >>
> >> Both the destination and source buffer are of fixed length
> >> so strscpy with 2-arguments is used.
> >>
> >> Introduce a typedef for cookie array to improve code clarity.
> >>
> >> Link: https://github.com/KSPP/linux/issues/88
> >> Signed-off-by: Siddarth Gundu <siddarthsgml@gmail.com>
> >> ---
> >> changes since v1
> >> - added a typedef for cookie arrays
> >>
> >> About the typedef: I was a bit hesitant to add it since the kernel
> >> style guide is against adding new typedef but I wanted to follow
> >> the review feedback for this.
> >
> > I personally think the typedef here is the appropriate.  But
> > it's really up to Ilya whether he likes this approach.  Get
> > his input before you do more.
>
> In any case, this should be 2 patches at that point, not collapsed
> into one patch.

Understood. If the typedef approach is a go after Ilya's feedback,
I'll split the changes into two separate patches as you suggested.
Thanks a lot for the review.

--
With Gratitude
Siddarth Gundu

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.