Re: [PATCH] quota: set init_needed flag only when successfully getting dquot

[cgxu519]

On 4/30/19 5:49 AM, Jan Kara wrote:

On Sun 28-04-19 13:39:21, Chengguang Xu wrote:

Set init_needed flag only when successfully getting dquot,
so that we can skip unnecessary subsequent operation.

Signed-off-by: Chengguang Xu 

Thanks for the patch but I don't think it's really useful. It will be very
rare that we race with quotaoff of dqget() fails due to error. So the
additional overhead of iterating over dquots doesn't really matter in that
case.


Hi Jan,

Thanks for the comment, I got it.

Chengguang.

Re: [PATCH 2/2] chardev: showing minor range for chardev in the output of /proc/devices

[cgxu519]

On 2/12/19 11:20 PM, Greg KH wrote:

On Tue, Feb 12, 2019 at 11:18:22PM +0800, cgxu519 wrote:

On 2/12/19 5:02 PM, Greg KH wrote:

On Tue, Feb 12, 2019 at 04:47:39PM +0800, Chengguang Xu wrote:

Currently chardev allows to share major, showing
major with minor range for chardev will be more
helpful.

Signed-off-by: Chengguang Xu 
---
   fs/char_dev.c | 4 +++-
   1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/char_dev.c b/fs/char_dev.c
index b25b1da097d5..6f00acdeb308 100644
--- a/fs/char_dev.c
+++ b/fs/char_dev.c
@@ -55,7 +55,9 @@ void chrdev_show(struct seq_file *f, off_t offset)
mutex_lock(_lock);
for (cd = chrdevs[major_to_index(offset)]; cd; cd = cd->next) {
if (cd->major == offset)
-   seq_printf(f, "%3d %s\n", cd->major, cd->name);
+   seq_printf(f, "%3d %s (%u-%u)\n", cd->major, cd->name,
+  cd->baseminor,
+  cd->baseminor + cd->minorct - 1);

You are changing the format of a userspace file, what tools are going to
break when you do this?

I'll remove this part in V2. Do you have any idea how to get the minor
range info for particular major? Or adding a similar file to somewhere
under /sys is acceptable?

Why do you need to know the minor range?  What can userspace do with
this that actually matters?


Assume that when we try to load a driver module and fail with -EBUSY
because of minor range overlapping, then what can we do for this case?
we even don't know what range has occupied and what range is available.

Also, I think we can obviously notice range overlapping bugs by showing
all registered minor ranges.

Thanks,
Chengguang.

Re: [PATCH 2/2] chardev: showing minor range for chardev in the output of /proc/devices

[cgxu519]

On 2/12/19 5:02 PM, Greg KH wrote:

On Tue, Feb 12, 2019 at 04:47:39PM +0800, Chengguang Xu wrote:

Currently chardev allows to share major, showing
major with minor range for chardev will be more
helpful.

Signed-off-by: Chengguang Xu 
---
  fs/char_dev.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/char_dev.c b/fs/char_dev.c
index b25b1da097d5..6f00acdeb308 100644
--- a/fs/char_dev.c
+++ b/fs/char_dev.c
@@ -55,7 +55,9 @@ void chrdev_show(struct seq_file *f, off_t offset)
mutex_lock(_lock);
for (cd = chrdevs[major_to_index(offset)]; cd; cd = cd->next) {
if (cd->major == offset)
-   seq_printf(f, "%3d %s\n", cd->major, cd->name);
+   seq_printf(f, "%3d %s (%u-%u)\n", cd->major, cd->name,
+  cd->baseminor,
+  cd->baseminor + cd->minorct - 1);

You are changing the format of a userspace file, what tools are going to
break when you do this?


I'll remove this part in V2. Do you have any idea how to get the minor
range info for particular major? Or adding a similar file to somewhere
under /sys is acceptable?

Thanks

Re: [PATCH] mei: expand minor range when registering chrdev region

[cgxu519]

On 2/12/19 5:29 PM, Greg KH wrote:

On Tue, Feb 12, 2019 at 02:02:52PM +0800, Chengguang Xu wrote:

Actually, total amount of available minor number
for a single major is MINORMARK + 1. So expand
minor range when registering chrdev region.

Signed-off-by: Chengguang Xu 
---
  drivers/misc/mei/main.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/misc/mei/main.c b/drivers/misc/mei/main.c
index 87281b3695e6..3df54f1e1a8b 100644
--- a/drivers/misc/mei/main.c
+++ b/drivers/misc/mei/main.c
@@ -869,7 +869,7 @@ static const struct file_operations mei_fops = {
  
  static struct class *mei_class;

  static dev_t mei_devt;
-#define MEI_MAX_DEVS  MINORMASK
+#define MEI_MAX_DEVS  (MINORMASK + 1)

Why is this needed?  Have you really run out of that many minor nodes
for this driver?


Not really, practically maybe we cannot reach to the limit.
I was just curious why only one minor number left there and assumed
that was from a mistake(since I've seen similar mistake in other driver).
However, if it explicitly sets to MINORMASK for some reasons, then it's
better to keep as is.

Thanks

Re: [PATCH] mm: adjust max read count in generic_file_buffered_read()

[cgxu519]

On 08/07/2018 09:54 PM, Jan Kara wrote:

On Mon 06-08-18 15:59:27, Andrew Morton wrote:

On Mon, 6 Aug 2018 12:22:03 +0200 Jan Kara  wrote:


On Fri 20-07-18 16:14:29, Andrew Morton wrote:

On Thu, 19 Jul 2018 10:58:12 +0200 Jan Kara  wrote:


On Thu 19-07-18 16:17:26, Chengguang Xu wrote:

When we try to truncate read count in generic_file_buffered_read(),
should deliver (sb->s_maxbytes - offset) as maximum count not
sb->s_maxbytes itself.

Signed-off-by: Chengguang Xu 

Looks good to me. You can add:

Reviewed-by: Jan Kara 

Yup.

What are the runtime effects of this bug?

Good question. I think ->readpage() could be called for index beyond
maximum file size supported by the filesystem leading to weird filesystem
behavior due to overflows in internal calculations.


Sure.  But is it possible for userspace to trigger this behaviour?
Possibly all callers have already sanitized the arguments by this stage
in which case the statement is arguably redundant.

So I don't think there's any sanitization going on before
generic_file_buffered_read(). E.g. I don't see any s_maxbytes check on
ksys_read() -> vfs_read() -> __vfs_read() -> new_sync_read() ->
call_read_iter() -> generic_file_read_iter() ->
generic_file_buffered_read() path... However now thinking about this again:
We are guaranteed i_size is within s_maxbytes (places modifying i_size
are checking for this) and generic_file_buffered_read() stops when it
should read beyond i_size. So in the end I don't think there's any breakage
possible and the patch is not necessary?


I think most of time i_size is within s_maxbytes in local filesystem,
but consider network filesystem, write big file in 64bit client and
read in 32bit client, in this case maybe generic_file_buffered_read()
can read more than s_maxbytes, right?


Thanks,
Chengguang

Re: [PATCH] mm: adjust max read count in generic_file_buffered_read()

[cgxu519]

On 08/07/2018 09:54 PM, Jan Kara wrote:

On Mon 06-08-18 15:59:27, Andrew Morton wrote:

On Mon, 6 Aug 2018 12:22:03 +0200 Jan Kara  wrote:


On Fri 20-07-18 16:14:29, Andrew Morton wrote:

On Thu, 19 Jul 2018 10:58:12 +0200 Jan Kara  wrote:


On Thu 19-07-18 16:17:26, Chengguang Xu wrote:

When we try to truncate read count in generic_file_buffered_read(),
should deliver (sb->s_maxbytes - offset) as maximum count not
sb->s_maxbytes itself.

Signed-off-by: Chengguang Xu 

Looks good to me. You can add:

Reviewed-by: Jan Kara 

Yup.

What are the runtime effects of this bug?

Good question. I think ->readpage() could be called for index beyond
maximum file size supported by the filesystem leading to weird filesystem
behavior due to overflows in internal calculations.


Sure.  But is it possible for userspace to trigger this behaviour?
Possibly all callers have already sanitized the arguments by this stage
in which case the statement is arguably redundant.

So I don't think there's any sanitization going on before
generic_file_buffered_read(). E.g. I don't see any s_maxbytes check on
ksys_read() -> vfs_read() -> __vfs_read() -> new_sync_read() ->
call_read_iter() -> generic_file_read_iter() ->
generic_file_buffered_read() path... However now thinking about this again:
We are guaranteed i_size is within s_maxbytes (places modifying i_size
are checking for this) and generic_file_buffered_read() stops when it
should read beyond i_size. So in the end I don't think there's any breakage
possible and the patch is not necessary?


I think most of time i_size is within s_maxbytes in local filesystem,
but consider network filesystem, write big file in 64bit client and
read in 32bit client, in this case maybe generic_file_buffered_read()
can read more than s_maxbytes, right?


Thanks,
Chengguang

Re: [PATCH] kernel/power: cast PAGE_SIZE to int when comparing with error code

[cgxu519]

Hi Rafael,

Could you have a look at this simple patch?

Thanks,
Chengguang


On 06/25/2018 01:30 PM, Chengguang Xu wrote:

If PAGE_SIZE is unsigned type then negative error code will be
larger than PAGE_SIZE.

Signed-off-by: Chengguang Xu 
---
  kernel/power/swap.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/power/swap.c b/kernel/power/swap.c
index c2bcf97d24c8..d7f6c1a288d3 100644
--- a/kernel/power/swap.c
+++ b/kernel/power/swap.c
@@ -923,7 +923,7 @@ int swsusp_write(unsigned int flags)
}
memset(, 0, sizeof(struct snapshot_handle));
error = snapshot_read_next();
-   if (error < PAGE_SIZE) {
+   if (error < (int)PAGE_SIZE) {
if (error >= 0)
error = -EFAULT;
  
@@ -1483,7 +1483,7 @@ int swsusp_read(unsigned int *flags_p)
  
  	memset(, 0, sizeof(struct snapshot_handle));

error = snapshot_write_next();
-   if (error < PAGE_SIZE)
+   if (error < (int)PAGE_SIZE)
return error < 0 ? error : -EFAULT;
header = (struct swsusp_info *)data_of(snapshot);
error = get_swap_reader(, flags_p);

Re: [PATCH] kernel/power: cast PAGE_SIZE to int when comparing with error code

[cgxu519]

Hi Rafael,

Could you have a look at this simple patch?

Thanks,
Chengguang


On 06/25/2018 01:30 PM, Chengguang Xu wrote:

If PAGE_SIZE is unsigned type then negative error code will be
larger than PAGE_SIZE.

Signed-off-by: Chengguang Xu 
---
  kernel/power/swap.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/power/swap.c b/kernel/power/swap.c
index c2bcf97d24c8..d7f6c1a288d3 100644
--- a/kernel/power/swap.c
+++ b/kernel/power/swap.c
@@ -923,7 +923,7 @@ int swsusp_write(unsigned int flags)
}
memset(, 0, sizeof(struct snapshot_handle));
error = snapshot_read_next();
-   if (error < PAGE_SIZE) {
+   if (error < (int)PAGE_SIZE) {
if (error >= 0)
error = -EFAULT;
  
@@ -1483,7 +1483,7 @@ int swsusp_read(unsigned int *flags_p)
  
  	memset(, 0, sizeof(struct snapshot_handle));

error = snapshot_write_next();
-   if (error < PAGE_SIZE)
+   if (error < (int)PAGE_SIZE)
return error < 0 ? error : -EFAULT;
header = (struct swsusp_info *)data_of(snapshot);
error = get_swap_reader(, flags_p);

Re: [PATCH 1/2] fs/exofs: fix potential memory leak in mount option parsing

[cgxu519]

Hi Boaz,

Could you have a look at this trivial patch?

Thanks,
Chengguang.

On 06/13/2018 12:05 PM, Chengguang Xu wrote:

There are some cases can cause memory leak when parsing
option 'osdname'.

Signed-off-by: Chengguang Xu 
---
  fs/exofs/super.c | 5 -
  1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/exofs/super.c b/fs/exofs/super.c
index 719a315..f3e17a9 100644
--- a/fs/exofs/super.c
+++ b/fs/exofs/super.c
@@ -101,6 +101,7 @@ static int parse_options(char *options, struct 
exofs_mountopt *opts)
token = match_token(p, tokens, args);
switch (token) {
case Opt_name:
+   kfree(opts->dev_name);
opts->dev_name = match_strdup([0]);
if (unlikely(!opts->dev_name)) {
EXOFS_ERR("Error allocating dev_name");
@@ -867,8 +868,10 @@ static struct dentry *exofs_mount(struct file_system_type 
*type,
int ret;
  
  	ret = parse_options(data, );

-   if (ret)
+   if (ret) {
+   kfree(opts.dev_name);
return ERR_PTR(ret);
+   }
  
  	if (!opts.dev_name)

opts.dev_name = dev_name;

Re: [PATCH 1/2] fs/exofs: fix potential memory leak in mount option parsing

[cgxu519]

Hi Boaz,

Could you have a look at this trivial patch?

Thanks,
Chengguang.

On 06/13/2018 12:05 PM, Chengguang Xu wrote:

There are some cases can cause memory leak when parsing
option 'osdname'.

Signed-off-by: Chengguang Xu 
---
  fs/exofs/super.c | 5 -
  1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/exofs/super.c b/fs/exofs/super.c
index 719a315..f3e17a9 100644
--- a/fs/exofs/super.c
+++ b/fs/exofs/super.c
@@ -101,6 +101,7 @@ static int parse_options(char *options, struct 
exofs_mountopt *opts)
token = match_token(p, tokens, args);
switch (token) {
case Opt_name:
+   kfree(opts->dev_name);
opts->dev_name = match_strdup([0]);
if (unlikely(!opts->dev_name)) {
EXOFS_ERR("Error allocating dev_name");
@@ -867,8 +868,10 @@ static struct dentry *exofs_mount(struct file_system_type 
*type,
int ret;
  
  	ret = parse_options(data, );

-   if (ret)
+   if (ret) {
+   kfree(opts.dev_name);
return ERR_PTR(ret);
+   }
  
  	if (!opts.dev_name)

opts.dev_name = dev_name;

Re: [V9fs-developer] [PATCH 2/2] fs/9p: detecting invalid options as much as possible

[cgxu519]

Hi Dominique,

Thanks for your quick reply and comment.


在 2018年4月16日，下午3:56，Dominique Martinet  写道：
> 
> Chengguang Xu wrote on Mon, Apr 16, 2018:
>>  default:
>> +p9_debug(P9_DEBUG_ERROR,
>> +"unrecognized mount option \"%s\" or missing 
>> value\n",
>> +p);
>>  continue;
> 
> The problem with that is that the same options are passed to the vfs,
> net and transport init later on - none of which know the full subset of
> valid options to tell what option has been recognized or not.
> 
> Unless we do some backward-incompatible breakage of passing all the
> net/transport options in its own option (e.g. net=foo:bar:moo) there
> unfortunately is no nice way of fixing this now.

Yes, I agree with you.


> 
> 
> (I don't mind the rest of the patches, although I'd say if we hit ENOMEM
> there is likely trouble going on so I'm not so sure about hiding it if
> there also is an EINVAL, but I don't really care)

The initial motivation of hiding ENOMEM here is when reproducing the error，
the error code may change by system condition(more accurately memory condition),
after this patch the error code will be persistent. However, as you pointed out,
when we hit ENOMEM there would be a serious trouble, so in real life maybe we
cannot benefit from it. What do you think? 

Thanks,
Chengguang.

Re: [V9fs-developer] [PATCH 2/2] fs/9p: detecting invalid options as much as possible

[cgxu519]

Hi Dominique,

Thanks for your quick reply and comment.


在 2018年4月16日，下午3:56，Dominique Martinet  写道：
> 
> Chengguang Xu wrote on Mon, Apr 16, 2018:
>>  default:
>> +p9_debug(P9_DEBUG_ERROR,
>> +"unrecognized mount option \"%s\" or missing 
>> value\n",
>> +p);
>>  continue;
> 
> The problem with that is that the same options are passed to the vfs,
> net and transport init later on - none of which know the full subset of
> valid options to tell what option has been recognized or not.
> 
> Unless we do some backward-incompatible breakage of passing all the
> net/transport options in its own option (e.g. net=foo:bar:moo) there
> unfortunately is no nice way of fixing this now.

Yes, I agree with you.


> 
> 
> (I don't mind the rest of the patches, although I'd say if we hit ENOMEM
> there is likely trouble going on so I'm not so sure about hiding it if
> there also is an EINVAL, but I don't really care)

The initial motivation of hiding ENOMEM here is when reproducing the error，
the error code may change by system condition(more accurately memory condition),
after this patch the error code will be persistent. However, as you pointed out,
when we hit ENOMEM there would be a serious trouble, so in real life maybe we
cannot benefit from it. What do you think? 

Thanks,
Chengguang.