Use bigger nodestr to accomodate 32-bit node numbers

Commit Message

Goldwyn Rodrigues Aug. 28, 2013, 4:15 a.m. UTC

While using pacemaker/corosync, the node numbers are generated using
IP address as opposed to serial node number generation. This may
not fit in a 8-byte string. Use a bigger string to print the 
complete node number.

Signed-off-by: Goldwyn Rodrigues <[e-mail]>
--- 
diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c
index 854d809..bc947e0 100644

Comments

jeff.liu Aug. 29, 2013, 9:52 a.m. UTC | #1

Hi Goldwyn,

On 08/28/2013 12:15 PM, Goldwyn Rodrigues wrote:

> While using pacemaker/corosync, the node numbers are generated using
> IP address as opposed to serial node number generation. This may
> not fit in a 8-byte string. Use a bigger string to print the 
> complete node number.

I have no experience of pacemaker/corosync, so here is my comments
pointing against this change only if we would expand the nodestr. :)

> 
> Signed-off-by: Goldwyn Rodrigues <[e-mail]>
> --- 
> diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c
> index 854d809..bc947e0 100644
> --- a/fs/ocfs2/super.c
> +++ b/fs/ocfs2/super.c
> @@ -1022,7 +1022,7 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent)
>  	struct inode *inode = NULL;
>  	struct ocfs2_super *osb = NULL;
>  	struct buffer_head *bh = NULL;
> -	char nodestr[8];
> +	char nodestr[12];

We have the corresponding debugging logic at ocfs2_dismount_volume(), which
defines the nodestr with the old size, it seems that this change would cause
potential buffer overflow while restore a bigger nodestr at it.

Also, there might has another relationship to ocfs2_control_message_setn/down
at stack_user.c because we limit the length of nodenum with a marco:
#define OCFS2_CONTROL_MESSAGE_NODENUM_LEN       8

Maybe we should unify them?

Thanks,
-Jeff

Goldwyn Rodrigues Aug. 29, 2013, 12:15 p.m. UTC | #2

On 08/29/2013 04:52 AM, Jeff Liu wrote:
> Hi Goldwyn,
>
> On 08/28/2013 12:15 PM, Goldwyn Rodrigues wrote:
>
>> While using pacemaker/corosync, the node numbers are generated using
>> IP address as opposed to serial node number generation. This may
>> not fit in a 8-byte string. Use a bigger string to print the
>> complete node number.
>
> I have no experience of pacemaker/corosync, so here is my comments
> pointing against this change only if we would expand the nodestr. :)
>
>>
>> Signed-off-by: Goldwyn Rodrigues <[e-mail]>
>> ---
>> diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c
>> index 854d809..bc947e0 100644
>> --- a/fs/ocfs2/super.c
>> +++ b/fs/ocfs2/super.c
>> @@ -1022,7 +1022,7 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent)
>>   	struct inode *inode = NULL;
>>   	struct ocfs2_super *osb = NULL;
>>   	struct buffer_head *bh = NULL;
>> -	char nodestr[8];
>> +	char nodestr[12];
>
> We have the corresponding debugging logic at ocfs2_dismount_volume(), which
> defines the nodestr with the old size, it seems that this change would cause
> potential buffer overflow while restore a bigger nodestr at it.

Yes, you are right. I will add the change in ocfs2_dismount_volume()  as 
well. Sorry, I missed this.

Andrew: Would you prefer a combined patch or a separate one?

>
> Also, there might has another relationship to ocfs2_control_message_setn/down
> at stack_user.c because we limit the length of nodenum with a marco:
> #define OCFS2_CONTROL_MESSAGE_NODENUM_LEN       8
>
> Maybe we should unify them?

This is a part of protocol to talk to the control daemon so we might not 
be able to just change this constant with changing other aspects such as 
the tools and the protocol versioning.

I am trying to get rid of control daemon handling altogether. Check my 
patches on "nocontrold". We are waiting for some review there. If that 
works, we don't need to change this constant ;)

Andrew Morton Aug. 29, 2013, 7:26 p.m. UTC | #3

On Thu, 29 Aug 2013 07:15:00 -0500 Goldwyn Rodrigues <[e-mail]> wrote:

> > We have the corresponding debugging logic at ocfs2_dismount_volume(), which
> > defines the nodestr with the old size, it seems that this change would cause
> > potential buffer overflow while restore a bigger nodestr at it.
> 
> Yes, you are right. I will add the change in ocfs2_dismount_volume()  as 
> well. Sorry, I missed this.
> 
> Andrew: Would you prefer a combined patch or a separate one?
> 

It will need to be a separate patch, please.

Patch

--- a/fs/ocfs2/super.c
+++ b/fs/ocfs2/super.c
@@ -1022,7 +1022,7 @@  static int ocfs2_fill_super(struct super_block *sb, void *data, int silent)
 	struct inode *inode = NULL;
 	struct ocfs2_super *osb = NULL;
 	struct buffer_head *bh = NULL;
-	char nodestr[8];
+	char nodestr[12];
 	struct ocfs2_blockcheck_stats stats;
 
 	trace_ocfs2_fill_super(sb, data, silent);