Re: [PATCH] arch:powerpc simple_write_to_buffer return check
On 05/02/21 12:51 pm, Christophe Leroy wrote: > Please provide some description of the change. > > And please clarify the patch subject, because as far as I can see, the return > is already checked allthough the check seams wrong. This was my first patch. I will try to provide better description of changes and subject in later patches. > Le 04/02/2021 à 19:16, Mayank Suman a écrit : >> Signed-off-by: Mayank Suman >> --- >> arch/powerpc/kernel/eeh.c | 8 >> arch/powerpc/platforms/powernv/eeh-powernv.c | 4 ++-- >> 2 files changed, 6 insertions(+), 6 deletions(-) >> >> diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c >> index 813713c9120c..2dbe1558a71f 100644 >> --- a/arch/powerpc/kernel/eeh.c >> +++ b/arch/powerpc/kernel/eeh.c >> @@ -1628,8 +1628,8 @@ static ssize_t eeh_force_recover_write(struct file >> *filp, >> char buf[20]; >> int ret; >> - ret = simple_write_to_buffer(buf, sizeof(buf), ppos, user_buf, count); >> - if (!ret) >> + ret = simple_write_to_buffer(buf, sizeof(buf)-1, ppos, user_buf, count); >> + if (ret <= 0) > return -EFAULT; > > Why return -EFAULT when the function has returned -EINVAL ? If -EINVAL is returned by simple_write_to_buffer, we should return -EINVAL. > And why is it -EFAULT when ret is 0 ? EFAULT means error accessing memory. > The earlier check returned EFAULT when ret is 0. Most probably, there was an assumption that writing 0 bytes (by simple_write_to_buffer) means a fault with memory (or error accessing memory).
Re: [PATCH] arch:powerpc simple_write_to_buffer return check
Please provide some description of the change. And please clarify the patch subject, because as far as I can see, the return is already checked allthough the check seams wrong. Le 04/02/2021 à 19:16, Mayank Suman a écrit : Signed-off-by: Mayank Suman --- arch/powerpc/kernel/eeh.c| 8 arch/powerpc/platforms/powernv/eeh-powernv.c | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c index 813713c9120c..2dbe1558a71f 100644 --- a/arch/powerpc/kernel/eeh.c +++ b/arch/powerpc/kernel/eeh.c @@ -1628,8 +1628,8 @@ static ssize_t eeh_force_recover_write(struct file *filp, char buf[20]; int ret; - ret = simple_write_to_buffer(buf, sizeof(buf), ppos, user_buf, count); - if (!ret) + ret = simple_write_to_buffer(buf, sizeof(buf)-1, ppos, user_buf, count); + if (ret <= 0) > return -EFAULT; Why return -EFAULT when the function has returned -EINVAL ? And why is it -EFAULT when ret is 0 ? EFAULT means error accessing memory. /* @@ -1696,7 +1696,7 @@ static ssize_t eeh_dev_check_write(struct file *filp, memset(buf, 0, sizeof(buf)); ret = simple_write_to_buffer(buf, sizeof(buf)-1, ppos, user_buf, count); - if (!ret) + if (ret <= 0) return -EFAULT; ret = sscanf(buf, "%x:%x:%x.%x", &domain, &bus, &dev, &fn); @@ -1836,7 +1836,7 @@ static ssize_t eeh_dev_break_write(struct file *filp, memset(buf, 0, sizeof(buf)); ret = simple_write_to_buffer(buf, sizeof(buf)-1, ppos, user_buf, count); - if (!ret) + if (ret <= 0) return -EFAULT; ret = sscanf(buf, "%x:%x:%x.%x", &domain, &bus, &dev, &fn); diff --git a/arch/powerpc/platforms/powernv/eeh-powernv.c b/arch/powerpc/platforms/powernv/eeh-powernv.c index 89e22c460ebf..36ed2b8f7375 100644 --- a/arch/powerpc/platforms/powernv/eeh-powernv.c +++ b/arch/powerpc/platforms/powernv/eeh-powernv.c @@ -76,8 +76,8 @@ static ssize_t pnv_eeh_ei_write(struct file *filp, return -ENXIO; /* Copy over argument buffer */ - ret = simple_write_to_buffer(buf, sizeof(buf), ppos, user_buf, count); - if (!ret) + ret = simple_write_to_buffer(buf, sizeof(buf)-1, ppos, user_buf, count); + if (ret <= 0) return -EFAULT; /* Retrieve parameters */
Re: [PATCH] arch:powerpc simple_write_to_buffer return check
On 05/02/21 4:05 am, Oliver O'Halloran wrote: > On Fri, Feb 5, 2021 at 5:17 AM Mayank Suman wrote: >> >> Signed-off-by: Mayank Suman > > commit messages aren't optional Sorry. I will include the commit message in PATCH v2. > >> --- >> arch/powerpc/kernel/eeh.c| 8 >> arch/powerpc/platforms/powernv/eeh-powernv.c | 4 ++-- >> 2 files changed, 6 insertions(+), 6 deletions(-) >> >> diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c >> index 813713c9120c..2dbe1558a71f 100644 >> --- a/arch/powerpc/kernel/eeh.c >> +++ b/arch/powerpc/kernel/eeh.c >> @@ -1628,8 +1628,8 @@ static ssize_t eeh_force_recover_write(struct file >> *filp, >> char buf[20]; >> int ret; >> >> - ret = simple_write_to_buffer(buf, sizeof(buf), ppos, user_buf, >> count); >> - if (!ret) >> + ret = simple_write_to_buffer(buf, sizeof(buf)-1, ppos, user_buf, >> count); > > We should probably be zeroing the buffer. Reading to sizeof(buf) - 1 > is done in a few places to guarantee that the string is nul > terminated, but without the preceeding memset() that isn't actually > guaranteed. Yes, the buffer should be zeroed out first. I have included memset() in Patch v2. > >> + if (ret <= 0) >> return -EFAULT; > > EFAULT is supposed to be returned when the user supplies a buffer to > write(2) which is outside their address space. I figured letting the > sscanf() in the next step fail if the user passes writes a zero-length > buffer and returning EINVAL made more sense. That said, the exact > semantics around zero length writes are pretty handwavy so I guess > this isn't wrong, but I don't think it's better either. > simple_write_to_buffer may return negative value on fail. So, -EFAULT should be return in case of negative return value. The conditional (!ret) was not sufficient to catch negative return value.
Re: [PATCH] arch:powerpc simple_write_to_buffer return check
On Fri, Feb 5, 2021 at 5:17 AM Mayank Suman wrote: > > Signed-off-by: Mayank Suman commit messages aren't optional > --- > arch/powerpc/kernel/eeh.c| 8 > arch/powerpc/platforms/powernv/eeh-powernv.c | 4 ++-- > 2 files changed, 6 insertions(+), 6 deletions(-) > > diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c > index 813713c9120c..2dbe1558a71f 100644 > --- a/arch/powerpc/kernel/eeh.c > +++ b/arch/powerpc/kernel/eeh.c > @@ -1628,8 +1628,8 @@ static ssize_t eeh_force_recover_write(struct file > *filp, > char buf[20]; > int ret; > > - ret = simple_write_to_buffer(buf, sizeof(buf), ppos, user_buf, count); > - if (!ret) > + ret = simple_write_to_buffer(buf, sizeof(buf)-1, ppos, user_buf, > count); We should probably be zeroing the buffer. Reading to sizeof(buf) - 1 is done in a few places to guarantee that the string is nul terminated, but without the preceeding memset() that isn't actually guaranteed. > + if (ret <= 0) > return -EFAULT; EFAULT is supposed to be returned when the user supplies a buffer to write(2) which is outside their address space. I figured letting the sscanf() in the next step fail if the user passes writes a zero-length buffer and returning EINVAL made more sense. That said, the exact semantics around zero length writes are pretty handwavy so I guess this isn't wrong, but I don't think it's better either. > /* > @@ -1696,7 +1696,7 @@ static ssize_t eeh_dev_check_write(struct file *filp, > > memset(buf, 0, sizeof(buf)); > ret = simple_write_to_buffer(buf, sizeof(buf)-1, ppos, user_buf, > count); > - if (!ret) > + if (ret <= 0) > return -EFAULT; > > ret = sscanf(buf, "%x:%x:%x.%x", &domain, &bus, &dev, &fn); > @@ -1836,7 +1836,7 @@ static ssize_t eeh_dev_break_write(struct file *filp, > > memset(buf, 0, sizeof(buf)); > ret = simple_write_to_buffer(buf, sizeof(buf)-1, ppos, user_buf, > count); > - if (!ret) > + if (ret <= 0) > return -EFAULT; > > ret = sscanf(buf, "%x:%x:%x.%x", &domain, &bus, &dev, &fn); > diff --git a/arch/powerpc/platforms/powernv/eeh-powernv.c > b/arch/powerpc/platforms/powernv/eeh-powernv.c > index 89e22c460ebf..36ed2b8f7375 100644 > --- a/arch/powerpc/platforms/powernv/eeh-powernv.c > +++ b/arch/powerpc/platforms/powernv/eeh-powernv.c > @@ -76,8 +76,8 @@ static ssize_t pnv_eeh_ei_write(struct file *filp, > return -ENXIO; > > /* Copy over argument buffer */ > - ret = simple_write_to_buffer(buf, sizeof(buf), ppos, user_buf, count); > - if (!ret) > + ret = simple_write_to_buffer(buf, sizeof(buf)-1, ppos, user_buf, > count); > + if (ret <= 0) > return -EFAULT; > > /* Retrieve parameters */ > -- > 2.30.0 >
[PATCH] arch:powerpc simple_write_to_buffer return check
Signed-off-by: Mayank Suman --- arch/powerpc/kernel/eeh.c| 8 arch/powerpc/platforms/powernv/eeh-powernv.c | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c index 813713c9120c..2dbe1558a71f 100644 --- a/arch/powerpc/kernel/eeh.c +++ b/arch/powerpc/kernel/eeh.c @@ -1628,8 +1628,8 @@ static ssize_t eeh_force_recover_write(struct file *filp, char buf[20]; int ret; - ret = simple_write_to_buffer(buf, sizeof(buf), ppos, user_buf, count); - if (!ret) + ret = simple_write_to_buffer(buf, sizeof(buf)-1, ppos, user_buf, count); + if (ret <= 0) return -EFAULT; /* @@ -1696,7 +1696,7 @@ static ssize_t eeh_dev_check_write(struct file *filp, memset(buf, 0, sizeof(buf)); ret = simple_write_to_buffer(buf, sizeof(buf)-1, ppos, user_buf, count); - if (!ret) + if (ret <= 0) return -EFAULT; ret = sscanf(buf, "%x:%x:%x.%x", &domain, &bus, &dev, &fn); @@ -1836,7 +1836,7 @@ static ssize_t eeh_dev_break_write(struct file *filp, memset(buf, 0, sizeof(buf)); ret = simple_write_to_buffer(buf, sizeof(buf)-1, ppos, user_buf, count); - if (!ret) + if (ret <= 0) return -EFAULT; ret = sscanf(buf, "%x:%x:%x.%x", &domain, &bus, &dev, &fn); diff --git a/arch/powerpc/platforms/powernv/eeh-powernv.c b/arch/powerpc/platforms/powernv/eeh-powernv.c index 89e22c460ebf..36ed2b8f7375 100644 --- a/arch/powerpc/platforms/powernv/eeh-powernv.c +++ b/arch/powerpc/platforms/powernv/eeh-powernv.c @@ -76,8 +76,8 @@ static ssize_t pnv_eeh_ei_write(struct file *filp, return -ENXIO; /* Copy over argument buffer */ - ret = simple_write_to_buffer(buf, sizeof(buf), ppos, user_buf, count); - if (!ret) + ret = simple_write_to_buffer(buf, sizeof(buf)-1, ppos, user_buf, count); + if (ret <= 0) return -EFAULT; /* Retrieve parameters */ -- 2.30.0
