[PATCH v3 0/4] platform/x86: dell_rbu: Packet data fixes

[PATCH v3 0/4] platform/x86: dell_rbu: Packet data fixes

[Stuart Hayes]

Make two fixes related to packet data and fix a sparse warning.

Signed-off-by: Stuart Hayes <stuart.w.hayes@gmail.com>

---

v3:
 -  Cosmetic changes to commit messages
 -  Bump module version

v2:
 -  Add "__must_hold" to create_packet to remove sparse warning.
 -  Make struct packet_data_list static.

Stuart Hayes (4):
  platform/x86: dell_rbu: Fix sparse warning
  platform/x86: dell_rbu: Fix list usage
  platform/x86: dell_rbu: Stop overwriting data buffer
  platform/x86: dell_rbu: Bump version

 drivers/platform/x86/dell/dell_rbu.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

-- 
2.47.1

[PATCH v3 1/4] platform/x86: dell_rbu: Fix lock context warning

[Stuart Hayes]

Fix a sparse lock context warning.

Signed-off-by: Stuart Hayes <stuart.w.hayes@gmail.com>
---
 drivers/platform/x86/dell/dell_rbu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/platform/x86/dell/dell_rbu.c b/drivers/platform/x86/dell/dell_rbu.c
index e30ca325938c..7b019fb72e86 100644
--- a/drivers/platform/x86/dell/dell_rbu.c
+++ b/drivers/platform/x86/dell/dell_rbu.c
@@ -91,7 +91,7 @@ static void init_packet_head(void)
 	rbu_data.imagesize = 0;
 }
 
-static int create_packet(void *data, size_t length)
+static int create_packet(void *data, size_t length) __must_hold(&rbu_data.lock)
 {
 	struct packet_data *newpacket;
 	int ordernum = 0;
-- 
2.47.1

[PATCH v3 2/4] platform/x86: dell_rbu: Fix list usage

[Stuart Hayes]

Stop using an entire struct packet_data just for the embedded list_head,
and fix usage of that list_head.

Fixes: d19f359fbdc6 ("platform/x86: dell_rbu: don't open code list_for_each_entry*()")
Signed-off-by: Stuart Hayes <stuart.w.hayes@gmail.com>
---
 drivers/platform/x86/dell/dell_rbu.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/platform/x86/dell/dell_rbu.c b/drivers/platform/x86/dell/dell_rbu.c
index 7b019fb72e86..c03d4d55fcc1 100644
--- a/drivers/platform/x86/dell/dell_rbu.c
+++ b/drivers/platform/x86/dell/dell_rbu.c
@@ -77,14 +77,14 @@ struct packet_data {
 	int ordernum;
 };
 
-static struct packet_data packet_data_head;
+static struct list_head packet_data_list;
 
 static struct platform_device *rbu_device;
 static int context;
 
 static void init_packet_head(void)
 {
-	INIT_LIST_HEAD(&packet_data_head.list);
+	INIT_LIST_HEAD(&packet_data_list);
 	rbu_data.packet_read_count = 0;
 	rbu_data.num_packets = 0;
 	rbu_data.packetsize = 0;
@@ -183,7 +183,7 @@ static int create_packet(void *data, size_t length) __must_hold(&rbu_data.lock)
 
 	/* initialize the newly created packet headers */
 	INIT_LIST_HEAD(&newpacket->list);
-	list_add_tail(&newpacket->list, &packet_data_head.list);
+	list_add_tail(&newpacket->list, &packet_data_list);
 
 	memcpy(newpacket->data, data, length);
 
@@ -292,7 +292,7 @@ static int packet_read_list(char *data, size_t * pread_length)
 	remaining_bytes = *pread_length;
 	bytes_read = rbu_data.packet_read_count;
 
-	list_for_each_entry(newpacket, (&packet_data_head.list)->next, list) {
+	list_for_each_entry(newpacket, &packet_data_list, list) {
 		bytes_copied = do_packet_read(pdest, newpacket,
 			remaining_bytes, bytes_read, &temp_count);
 		remaining_bytes -= bytes_copied;
@@ -315,7 +315,7 @@ static void packet_empty_list(void)
 {
 	struct packet_data *newpacket, *tmp;
 
-	list_for_each_entry_safe(newpacket, tmp, (&packet_data_head.list)->next, list) {
+	list_for_each_entry_safe(newpacket, tmp, &packet_data_list, list) {
 		list_del(&newpacket->list);
 
 		/*
-- 
2.47.1

[PATCH v3 3/4] platform/x86: dell_rbu: Stop overwriting data buffer

[Stuart Hayes]

The dell_rbu driver will use memset() to clear the data held by each
packet when it is no longer needed (when the driver is unloaded, the
packet size is changed, etc).

The amount of memory that is cleared (before this patch) is the normal
packet size. However, the last packet in the list may be smaller.

Fix this to only clear the memory actually used by each packet, to prevent
it from writing past the end of data buffer.

Signed-off-by: Stuart Hayes <stuart.w.hayes@gmail.com>
---
 drivers/platform/x86/dell/dell_rbu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/platform/x86/dell/dell_rbu.c b/drivers/platform/x86/dell/dell_rbu.c
index c03d4d55fcc1..7d5b26735a20 100644
--- a/drivers/platform/x86/dell/dell_rbu.c
+++ b/drivers/platform/x86/dell/dell_rbu.c
@@ -322,7 +322,7 @@ static void packet_empty_list(void)
 		 * zero out the RBU packet memory before freeing
 		 * to make sure there are no stale RBU packets left in memory
 		 */
-		memset(newpacket->data, 0, rbu_data.packetsize);
+		memset(newpacket->data, 0, newpacket->length);
 		set_memory_wb((unsigned long)newpacket->data,
 			1 << newpacket->ordernum);
 		free_pages((unsigned long) newpacket->data,
-- 
2.47.1

[PATCH v3 4/4] platform/x86: dell_rbu: Bump version

[Stuart Hayes]

Bump the module version.

Signed-off-by: Stuart Hayes <stuart.w.hayes@gmail.com>
---
 drivers/platform/x86/dell/dell_rbu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/platform/x86/dell/dell_rbu.c b/drivers/platform/x86/dell/dell_rbu.c
index 7d5b26735a20..45c0a72e494a 100644
--- a/drivers/platform/x86/dell/dell_rbu.c
+++ b/drivers/platform/x86/dell/dell_rbu.c
@@ -45,7 +45,7 @@
 MODULE_AUTHOR("Abhay Salunke <abhay_salunke@dell.com>");
 MODULE_DESCRIPTION("Driver for updating BIOS image on DELL systems");
 MODULE_LICENSE("GPL");
-MODULE_VERSION("3.2");
+MODULE_VERSION("3.3");
 
 #define BIOS_SCAN_LIMIT 0xffffffff
 #define MAX_IMAGE_LENGTH 16
-- 
2.47.1

Re: [PATCH v3 2/4] platform/x86: dell_rbu: Fix list usage

[Ilpo Järvinen]

On Thu, 29 May 2025, Stuart Hayes wrote:

> Stop using an entire struct packet_data just for the embedded list_head,
> and fix usage of that list_head.
> 
> Fixes: d19f359fbdc6 ("platform/x86: dell_rbu: don't open code list_for_each_entry*()")
> Signed-off-by: Stuart Hayes <stuart.w.hayes@gmail.com>

Isn't this just refactor so Fixes tag for this commit is not warranted?

> ---
>  drivers/platform/x86/dell/dell_rbu.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/platform/x86/dell/dell_rbu.c b/drivers/platform/x86/dell/dell_rbu.c
> index 7b019fb72e86..c03d4d55fcc1 100644
> --- a/drivers/platform/x86/dell/dell_rbu.c
> +++ b/drivers/platform/x86/dell/dell_rbu.c
> @@ -77,14 +77,14 @@ struct packet_data {
>  	int ordernum;
>  };
>  
> -static struct packet_data packet_data_head;
> +static struct list_head packet_data_list;
>  
>  static struct platform_device *rbu_device;
>  static int context;
>  
>  static void init_packet_head(void)
>  {
> -	INIT_LIST_HEAD(&packet_data_head.list);
> +	INIT_LIST_HEAD(&packet_data_list);
>  	rbu_data.packet_read_count = 0;
>  	rbu_data.num_packets = 0;
>  	rbu_data.packetsize = 0;
> @@ -183,7 +183,7 @@ static int create_packet(void *data, size_t length) __must_hold(&rbu_data.lock)
>  
>  	/* initialize the newly created packet headers */
>  	INIT_LIST_HEAD(&newpacket->list);
> -	list_add_tail(&newpacket->list, &packet_data_head.list);
> +	list_add_tail(&newpacket->list, &packet_data_list);
>  
>  	memcpy(newpacket->data, data, length);
>  
> @@ -292,7 +292,7 @@ static int packet_read_list(char *data, size_t * pread_length)
>  	remaining_bytes = *pread_length;
>  	bytes_read = rbu_data.packet_read_count;
>  
> -	list_for_each_entry(newpacket, (&packet_data_head.list)->next, list) {
> +	list_for_each_entry(newpacket, &packet_data_list, list) {
>  		bytes_copied = do_packet_read(pdest, newpacket,
>  			remaining_bytes, bytes_read, &temp_count);
>  		remaining_bytes -= bytes_copied;
> @@ -315,7 +315,7 @@ static void packet_empty_list(void)
>  {
>  	struct packet_data *newpacket, *tmp;
>  
> -	list_for_each_entry_safe(newpacket, tmp, (&packet_data_head.list)->next, list) {
> +	list_for_each_entry_safe(newpacket, tmp, &packet_data_list, list) {
>  		list_del(&newpacket->list);
>  
>  		/*
> 

-- 
 i.

Re: [PATCH v3 3/4] platform/x86: dell_rbu: Stop overwriting data buffer

[Ilpo Järvinen]

On Thu, 29 May 2025, Stuart Hayes wrote:

> The dell_rbu driver will use memset() to clear the data held by each
> packet when it is no longer needed (when the driver is unloaded, the
> packet size is changed, etc).
> 
> The amount of memory that is cleared (before this patch) is the normal
> packet size. However, the last packet in the list may be smaller.
> 
> Fix this to only clear the memory actually used by each packet, to prevent
> it from writing past the end of data buffer.
> 
> Signed-off-by: Stuart Hayes <stuart.w.hayes@gmail.com>

This still doesn't have Fixes tag? If it writes part the buffer, there 
certainly should be one in this one. Did you perhaps add it to a wrong 
patch?

> ---
>  drivers/platform/x86/dell/dell_rbu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/dell/dell_rbu.c b/drivers/platform/x86/dell/dell_rbu.c
> index c03d4d55fcc1..7d5b26735a20 100644
> --- a/drivers/platform/x86/dell/dell_rbu.c
> +++ b/drivers/platform/x86/dell/dell_rbu.c
> @@ -322,7 +322,7 @@ static void packet_empty_list(void)
>  		 * zero out the RBU packet memory before freeing
>  		 * to make sure there are no stale RBU packets left in memory
>  		 */
> -		memset(newpacket->data, 0, rbu_data.packetsize);
> +		memset(newpacket->data, 0, newpacket->length);
>  		set_memory_wb((unsigned long)newpacket->data,
>  			1 << newpacket->ordernum);
>  		free_pages((unsigned long) newpacket->data,
> 

-- 
 i.

Re: [PATCH v3 2/4] platform/x86: dell_rbu: Fix list usage

[stuart hayes]

On 5/30/2025 2:54 AM, Ilpo Järvinen wrote:
> On Thu, 29 May 2025, Stuart Hayes wrote:
> 
>> Stop using an entire struct packet_data just for the embedded list_head,
>> and fix usage of that list_head.
>>
>> Fixes: d19f359fbdc6 ("platform/x86: dell_rbu: don't open code list_for_each_entry*()")
>> Signed-off-by: Stuart Hayes <stuart.w.hayes@gmail.com>
> 
> Isn't this just refactor so Fixes tag for this commit is not warranted?
> 

No. The patch that this fixes had converted the driver to use 
list_for_each_entry*() to loop through the packet list instead of a 
while loop. But it passed (&packet_data_head.list)->next to 
list_for_each_entry*() instead of the list head itself.

That resulted in to issues. In the function that prints the packets, it 
would start with the wrong packet, and in the function that deletes the 
packets, it would get a null pointer dereference when it tried to zero 
out the data associated with the packet that held the actual list head.

>> ---
>>   drivers/platform/x86/dell/dell_rbu.c | 10 +++++-----
>>   1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/platform/x86/dell/dell_rbu.c b/drivers/platform/x86/dell/dell_rbu.c
>> index 7b019fb72e86..c03d4d55fcc1 100644
>> --- a/drivers/platform/x86/dell/dell_rbu.c
>> +++ b/drivers/platform/x86/dell/dell_rbu.c
>> @@ -77,14 +77,14 @@ struct packet_data {
>>   	int ordernum;
>>   };
>>   
>> -static struct packet_data packet_data_head;
>> +static struct list_head packet_data_list;
>>   
>>   static struct platform_device *rbu_device;
>>   static int context;
>>   
>>   static void init_packet_head(void)
>>   {
>> -	INIT_LIST_HEAD(&packet_data_head.list);
>> +	INIT_LIST_HEAD(&packet_data_list);
>>   	rbu_data.packet_read_count = 0;
>>   	rbu_data.num_packets = 0;
>>   	rbu_data.packetsize = 0;
>> @@ -183,7 +183,7 @@ static int create_packet(void *data, size_t length) __must_hold(&rbu_data.lock)
>>   
>>   	/* initialize the newly created packet headers */
>>   	INIT_LIST_HEAD(&newpacket->list);
>> -	list_add_tail(&newpacket->list, &packet_data_head.list);
>> +	list_add_tail(&newpacket->list, &packet_data_list);
>>   
>>   	memcpy(newpacket->data, data, length);
>>   
>> @@ -292,7 +292,7 @@ static int packet_read_list(char *data, size_t * pread_length)
>>   	remaining_bytes = *pread_length;
>>   	bytes_read = rbu_data.packet_read_count;
>>   
>> -	list_for_each_entry(newpacket, (&packet_data_head.list)->next, list) {
>> +	list_for_each_entry(newpacket, &packet_data_list, list) {
>>   		bytes_copied = do_packet_read(pdest, newpacket,
>>   			remaining_bytes, bytes_read, &temp_count);
>>   		remaining_bytes -= bytes_copied;
>> @@ -315,7 +315,7 @@ static void packet_empty_list(void)
>>   {
>>   	struct packet_data *newpacket, *tmp;
>>   
>> -	list_for_each_entry_safe(newpacket, tmp, (&packet_data_head.list)->next, list) {
>> +	list_for_each_entry_safe(newpacket, tmp, &packet_data_list, list) {
>>   		list_del(&newpacket->list);
>>   
>>   		/*
>>
> 

Re: [PATCH v3 2/4] platform/x86: dell_rbu: Fix list usage

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 3540 bytes --]

On Fri, 30 May 2025, stuart hayes wrote:

> On 5/30/2025 2:54 AM, Ilpo Järvinen wrote:
> > On Thu, 29 May 2025, Stuart Hayes wrote:
> > 
> > > Stop using an entire struct packet_data just for the embedded list_head,
> > > and fix usage of that list_head.
> > > 
> > > Fixes: d19f359fbdc6 ("platform/x86: dell_rbu: don't open code
> > > list_for_each_entry*()")
> > > Signed-off-by: Stuart Hayes <stuart.w.hayes@gmail.com>
> > 
> > Isn't this just refactor so Fixes tag for this commit is not warranted?
> > 
> 
> No. The patch that this fixes had converted the driver to use
> list_for_each_entry*() to loop through the packet list instead of a while
> loop. But it passed (&packet_data_head.list)->next to list_for_each_entry*()
> instead of the list head itself.
> 
> That resulted in to issues. In the function that prints the packets, it would
> start with the wrong packet, and in the function that deletes the packets, it
> would get a null pointer dereference when it tried to zero out the data
> associated with the packet that held the actual list head.

Oh, I see that difference now. Good catch.

However, that also means the ->next part is wrong and there are two 
independent changes here, one that fixes this ->next problem and then the 
refactoring of packet_data_head to packet_data_list?

> > > ---
> > >   drivers/platform/x86/dell/dell_rbu.c | 10 +++++-----
> > >   1 file changed, 5 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/platform/x86/dell/dell_rbu.c
> > > b/drivers/platform/x86/dell/dell_rbu.c
> > > index 7b019fb72e86..c03d4d55fcc1 100644
> > > --- a/drivers/platform/x86/dell/dell_rbu.c
> > > +++ b/drivers/platform/x86/dell/dell_rbu.c
> > > @@ -77,14 +77,14 @@ struct packet_data {
> > >   	int ordernum;
> > >   };
> > >   -static struct packet_data packet_data_head;
> > > +static struct list_head packet_data_list;
> > >     static struct platform_device *rbu_device;
> > >   static int context;
> > >     static void init_packet_head(void)
> > >   {
> > > -	INIT_LIST_HEAD(&packet_data_head.list);
> > > +	INIT_LIST_HEAD(&packet_data_list);
> > >   	rbu_data.packet_read_count = 0;
> > >   	rbu_data.num_packets = 0;
> > >   	rbu_data.packetsize = 0;
> > > @@ -183,7 +183,7 @@ static int create_packet(void *data, size_t length)
> > > __must_hold(&rbu_data.lock)
> > >     	/* initialize the newly created packet headers */
> > >   	INIT_LIST_HEAD(&newpacket->list);
> > > -	list_add_tail(&newpacket->list, &packet_data_head.list);
> > > +	list_add_tail(&newpacket->list, &packet_data_list);
> > >     	memcpy(newpacket->data, data, length);
> > >   @@ -292,7 +292,7 @@ static int packet_read_list(char *data, size_t *
> > > pread_length)
> > >   	remaining_bytes = *pread_length;
> > >   	bytes_read = rbu_data.packet_read_count;
> > >   -	list_for_each_entry(newpacket, (&packet_data_head.list)->next, list) {
> > > +	list_for_each_entry(newpacket, &packet_data_list, list) {
> > >   		bytes_copied = do_packet_read(pdest, newpacket,
> > >   			remaining_bytes, bytes_read, &temp_count);
> > >   		remaining_bytes -= bytes_copied;
> > > @@ -315,7 +315,7 @@ static void packet_empty_list(void)
> > >   {
> > >   	struct packet_data *newpacket, *tmp;
> > >   -	list_for_each_entry_safe(newpacket, tmp,
> > > (&packet_data_head.list)->next, list) {
> > > +	list_for_each_entry_safe(newpacket, tmp, &packet_data_list, list) {
> > >   		list_del(&newpacket->list);
> > >     		/*
> > > 
> > 
> 

-- 
 i.

Re: [PATCH v3 3/4] platform/x86: dell_rbu: Stop overwriting data buffer

[stuart hayes]

On 5/30/2025 3:03 AM, Ilpo Järvinen wrote:
> On Thu, 29 May 2025, Stuart Hayes wrote:
> 
>> The dell_rbu driver will use memset() to clear the data held by each
>> packet when it is no longer needed (when the driver is unloaded, the
>> packet size is changed, etc).
>>
>> The amount of memory that is cleared (before this patch) is the normal
>> packet size. However, the last packet in the list may be smaller.
>>
>> Fix this to only clear the memory actually used by each packet, to prevent
>> it from writing past the end of data buffer.
>>
>> Signed-off-by: Stuart Hayes <stuart.w.hayes@gmail.com>
> 
> This still doesn't have Fixes tag? If it writes part the buffer, there
> certainly should be one in this one. Did you perhaps add it to a wrong
> patch?
> 

This fixes a bug that's existed for the life of the driver. Should I use 
the patch that introduced the driver for "Fixes:"?

I don't think this has ever caused a problem, because, as far as I know, 
the Dell BIOS update program is the only user of this module, and it 
uses 4096 as the packet size. Because the packet data buffers are 
allocated with...

	ordernum = get_order(size);
	image_update_buffer =
		(unsigned char *)__get_free_pages(GRP_DMA32, ordernum);

...this will always allocate one full page for every packet data buffer.

I just happened to notice that the driver could overwrite the end of the 
buffer for the last packet if a packet size of more than 4096 was used, 
so I thought I'd fix that.

>> ---
>>   drivers/platform/x86/dell/dell_rbu.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/platform/x86/dell/dell_rbu.c b/drivers/platform/x86/dell/dell_rbu.c
>> index c03d4d55fcc1..7d5b26735a20 100644
>> --- a/drivers/platform/x86/dell/dell_rbu.c
>> +++ b/drivers/platform/x86/dell/dell_rbu.c
>> @@ -322,7 +322,7 @@ static void packet_empty_list(void)
>>   		 * zero out the RBU packet memory before freeing
>>   		 * to make sure there are no stale RBU packets left in memory
>>   		 */
>> -		memset(newpacket->data, 0, rbu_data.packetsize);
>> +		memset(newpacket->data, 0, newpacket->length);
>>   		set_memory_wb((unsigned long)newpacket->data,
>>   			1 << newpacket->ordernum);
>>   		free_pages((unsigned long) newpacket->data,
>>
> 

Re: [PATCH v3 2/4] platform/x86: dell_rbu: Fix list usage

[stuart hayes]

On 5/30/2025 10:25 AM, Ilpo Järvinen wrote:
> On Fri, 30 May 2025, stuart hayes wrote:
> 
>> On 5/30/2025 2:54 AM, Ilpo Järvinen wrote:
>>> On Thu, 29 May 2025, Stuart Hayes wrote:
>>>
>>>> Stop using an entire struct packet_data just for the embedded list_head,
>>>> and fix usage of that list_head.
>>>>
>>>> Fixes: d19f359fbdc6 ("platform/x86: dell_rbu: don't open code
>>>> list_for_each_entry*()")
>>>> Signed-off-by: Stuart Hayes <stuart.w.hayes@gmail.com>
>>>
>>> Isn't this just refactor so Fixes tag for this commit is not warranted?
>>>
>>
>> No. The patch that this fixes had converted the driver to use
>> list_for_each_entry*() to loop through the packet list instead of a while
>> loop. But it passed (&packet_data_head.list)->next to list_for_each_entry*()
>> instead of the list head itself.
>>
>> That resulted in to issues. In the function that prints the packets, it would
>> start with the wrong packet, and in the function that deletes the packets, it
>> would get a null pointer dereference when it tried to zero out the data
>> associated with the packet that held the actual list head.
> 
> Oh, I see that difference now. Good catch.
> 
> However, that also means the ->next part is wrong and there are two
> independent changes here, one that fixes this ->next problem and then the
> refactoring of packet_data_head to packet_data_list?
> 

Correct. Do you want those as two separate patches?

>>>> ---
>>>>    drivers/platform/x86/dell/dell_rbu.c | 10 +++++-----
>>>>    1 file changed, 5 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/platform/x86/dell/dell_rbu.c
>>>> b/drivers/platform/x86/dell/dell_rbu.c
>>>> index 7b019fb72e86..c03d4d55fcc1 100644
>>>> --- a/drivers/platform/x86/dell/dell_rbu.c
>>>> +++ b/drivers/platform/x86/dell/dell_rbu.c
>>>> @@ -77,14 +77,14 @@ struct packet_data {
>>>>    	int ordernum;
>>>>    };
>>>>    -static struct packet_data packet_data_head;
>>>> +static struct list_head packet_data_list;
>>>>      static struct platform_device *rbu_device;
>>>>    static int context;
>>>>      static void init_packet_head(void)
>>>>    {
>>>> -	INIT_LIST_HEAD(&packet_data_head.list);
>>>> +	INIT_LIST_HEAD(&packet_data_list);
>>>>    	rbu_data.packet_read_count = 0;
>>>>    	rbu_data.num_packets = 0;
>>>>    	rbu_data.packetsize = 0;
>>>> @@ -183,7 +183,7 @@ static int create_packet(void *data, size_t length)
>>>> __must_hold(&rbu_data.lock)
>>>>      	/* initialize the newly created packet headers */
>>>>    	INIT_LIST_HEAD(&newpacket->list);
>>>> -	list_add_tail(&newpacket->list, &packet_data_head.list);
>>>> +	list_add_tail(&newpacket->list, &packet_data_list);
>>>>      	memcpy(newpacket->data, data, length);
>>>>    @@ -292,7 +292,7 @@ static int packet_read_list(char *data, size_t *
>>>> pread_length)
>>>>    	remaining_bytes = *pread_length;
>>>>    	bytes_read = rbu_data.packet_read_count;
>>>>    -	list_for_each_entry(newpacket, (&packet_data_head.list)->next, list) {
>>>> +	list_for_each_entry(newpacket, &packet_data_list, list) {
>>>>    		bytes_copied = do_packet_read(pdest, newpacket,
>>>>    			remaining_bytes, bytes_read, &temp_count);
>>>>    		remaining_bytes -= bytes_copied;
>>>> @@ -315,7 +315,7 @@ static void packet_empty_list(void)
>>>>    {
>>>>    	struct packet_data *newpacket, *tmp;
>>>>    -	list_for_each_entry_safe(newpacket, tmp,
>>>> (&packet_data_head.list)->next, list) {
>>>> +	list_for_each_entry_safe(newpacket, tmp, &packet_data_list, list) {
>>>>    		list_del(&newpacket->list);
>>>>      		/*
>>>>
>>>
>>
> 

Re: [PATCH v3 3/4] platform/x86: dell_rbu: Stop overwriting data buffer

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 2941 bytes --]

On Fri, 30 May 2025, stuart hayes wrote:

> On 5/30/2025 3:03 AM, Ilpo Järvinen wrote:
> > On Thu, 29 May 2025, Stuart Hayes wrote:
> > 
> > > The dell_rbu driver will use memset() to clear the data held by each
> > > packet when it is no longer needed (when the driver is unloaded, the
> > > packet size is changed, etc).
> > > 
> > > The amount of memory that is cleared (before this patch) is the normal
> > > packet size. However, the last packet in the list may be smaller.
> > > 
> > > Fix this to only clear the memory actually used by each packet, to prevent
> > > it from writing past the end of data buffer.
> > > 
> > > Signed-off-by: Stuart Hayes <stuart.w.hayes@gmail.com>
> > 
> > This still doesn't have Fixes tag? If it writes part the buffer, there
> > certainly should be one in this one. Did you perhaps add it to a wrong
> > patch?
> > 
> 
> This fixes a bug that's existed for the life of the driver. Should I use the
> patch that introduced the driver for "Fixes:"?

Yes. They're normal commits just like every other commit. And since first 
commits are quite complicated, it's nothing unusual to find problems that 
have been there right from the beginning.

> I don't think this has ever caused a problem, because, as far as I know, the
> Dell BIOS update program is the only user of this module, and it uses 4096 as
> the packet size. Because the packet data buffers are allocated with...
> 
> 	ordernum = get_order(size);
> 	image_update_buffer =
> 		(unsigned char *)__get_free_pages(GRP_DMA32, ordernum);
> 
> ...this will always allocate one full page for every packet data buffer.
> 
> I just happened to notice that the driver could overwrite the end of the
> buffer for the last packet if a packet size of more than 4096 was used, so I
> thought I'd fix that.

It would be worth to mention enough these details then in the changelog so 
that other people such as stable maintainers can understand it so they can 
base their reasoning on facts provided rather than guesswork.

-- 
 i.


> > > ---
> > >   drivers/platform/x86/dell/dell_rbu.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/platform/x86/dell/dell_rbu.c
> > > b/drivers/platform/x86/dell/dell_rbu.c
> > > index c03d4d55fcc1..7d5b26735a20 100644
> > > --- a/drivers/platform/x86/dell/dell_rbu.c
> > > +++ b/drivers/platform/x86/dell/dell_rbu.c
> > > @@ -322,7 +322,7 @@ static void packet_empty_list(void)
> > >   		 * zero out the RBU packet memory before freeing
> > >   		 * to make sure there are no stale RBU packets left in memory
> > >   		 */
> > > -		memset(newpacket->data, 0, rbu_data.packetsize);
> > > +		memset(newpacket->data, 0, newpacket->length);
> > >   		set_memory_wb((unsigned long)newpacket->data,
> > >   			1 << newpacket->ordernum);
> > >   		free_pages((unsigned long) newpacket->data,
> > > 
> > 
> 

Re: [PATCH v3 2/4] platform/x86: dell_rbu: Fix list usage

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 4276 bytes --]

On Fri, 30 May 2025, stuart hayes wrote:

> On 5/30/2025 10:25 AM, Ilpo Järvinen wrote:
> > On Fri, 30 May 2025, stuart hayes wrote:
> > 
> > > On 5/30/2025 2:54 AM, Ilpo Järvinen wrote:
> > > > On Thu, 29 May 2025, Stuart Hayes wrote:
> > > > 
> > > > > Stop using an entire struct packet_data just for the embedded
> > > > > list_head,
> > > > > and fix usage of that list_head.
> > > > > 
> > > > > Fixes: d19f359fbdc6 ("platform/x86: dell_rbu: don't open code
> > > > > list_for_each_entry*()")
> > > > > Signed-off-by: Stuart Hayes <stuart.w.hayes@gmail.com>
> > > > 
> > > > Isn't this just refactor so Fixes tag for this commit is not warranted?
> > > > 
> > > 
> > > No. The patch that this fixes had converted the driver to use
> > > list_for_each_entry*() to loop through the packet list instead of a while
> > > loop. But it passed (&packet_data_head.list)->next to
> > > list_for_each_entry*()
> > > instead of the list head itself.
> > > 
> > > That resulted in to issues. In the function that prints the packets, it
> > > would
> > > start with the wrong packet, and in the function that deletes the packets,
> > > it
> > > would get a null pointer dereference when it tried to zero out the data
> > > associated with the packet that held the actual list head.
> > 
> > Oh, I see that difference now. Good catch.
> > 
> > However, that also means the ->next part is wrong and there are two
> > independent changes here, one that fixes this ->next problem and then the
> > refactoring of packet_data_head to packet_data_list?
> > 
> 
> Correct. Do you want those as two separate patches?

Yes please, and please order the fix patch before the refactor patch as 
stable people will be interested in taking the fix into stable releases.

-- 
 i.

> > > > > ---
> > > > >    drivers/platform/x86/dell/dell_rbu.c | 10 +++++-----
> > > > >    1 file changed, 5 insertions(+), 5 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/platform/x86/dell/dell_rbu.c
> > > > > b/drivers/platform/x86/dell/dell_rbu.c
> > > > > index 7b019fb72e86..c03d4d55fcc1 100644
> > > > > --- a/drivers/platform/x86/dell/dell_rbu.c
> > > > > +++ b/drivers/platform/x86/dell/dell_rbu.c
> > > > > @@ -77,14 +77,14 @@ struct packet_data {
> > > > >    	int ordernum;
> > > > >    };
> > > > >    -static struct packet_data packet_data_head;
> > > > > +static struct list_head packet_data_list;
> > > > >      static struct platform_device *rbu_device;
> > > > >    static int context;
> > > > >      static void init_packet_head(void)
> > > > >    {
> > > > > -	INIT_LIST_HEAD(&packet_data_head.list);
> > > > > +	INIT_LIST_HEAD(&packet_data_list);
> > > > >    	rbu_data.packet_read_count = 0;
> > > > >    	rbu_data.num_packets = 0;
> > > > >    	rbu_data.packetsize = 0;
> > > > > @@ -183,7 +183,7 @@ static int create_packet(void *data, size_t
> > > > > length)
> > > > > __must_hold(&rbu_data.lock)
> > > > >      	/* initialize the newly created packet headers */
> > > > >    	INIT_LIST_HEAD(&newpacket->list);
> > > > > -	list_add_tail(&newpacket->list, &packet_data_head.list);
> > > > > +	list_add_tail(&newpacket->list, &packet_data_list);
> > > > >      	memcpy(newpacket->data, data, length);
> > > > >    @@ -292,7 +292,7 @@ static int packet_read_list(char *data, size_t
> > > > > *
> > > > > pread_length)
> > > > >    	remaining_bytes = *pread_length;
> > > > >    	bytes_read = rbu_data.packet_read_count;
> > > > >    -	list_for_each_entry(newpacket, (&packet_data_head.list)->next,
> > > > > list) {
> > > > > +	list_for_each_entry(newpacket, &packet_data_list, list) {
> > > > >    		bytes_copied = do_packet_read(pdest, newpacket,
> > > > >    			remaining_bytes, bytes_read, &temp_count);
> > > > >    		remaining_bytes -= bytes_copied;
> > > > > @@ -315,7 +315,7 @@ static void packet_empty_list(void)
> > > > >    {
> > > > >    	struct packet_data *newpacket, *tmp;
> > > > >    -	list_for_each_entry_safe(newpacket, tmp,
> > > > > (&packet_data_head.list)->next, list) {
> > > > > +	list_for_each_entry_safe(newpacket, tmp, &packet_data_list,
> > > > > list) {
> > > > >    		list_del(&newpacket->list);
> > > > >      		/*
> > > > > 
> > > > 
> > > 
> > 
>