* [PATCH] zcache: fix "zcache=" kernel parameter
@ 2013-07-19 14:46 Piotr Sarna
2013-07-19 15:20 ` Konrad Rzeszutek Wilk
2013-07-24 10:02 ` Michal Hocko
0 siblings, 2 replies; 13+ messages in thread
From: Piotr Sarna @ 2013-07-19 14:46 UTC (permalink / raw)
To: konrad.wilk
Cc: bob.liu, gregkh, linux-kernel, devel, b.zolnierkie, Piotr Sarna,
Kyungmin Park
Commit 835f2f5 ("staging: zcache: enable zcache to be built/loaded as
a module") introduced an incorrect handling of "zcache=" parameter.
Inside zcache_comp_init() function, zcache_comp_name variable is
checked for being empty. If not empty, the above variable is tested
for being compatible with Crypto API. Unfortunately, after that
function ends unconditionally (by the "goto out" directive) and returns:
- non-zero value if verification succeeded, wrongly indicating an error
- zero value if verification failed, falsely informing that function
zcache_comp_init() ended properly.
A solution to this problem is as following:
1. Move the "goto out" directive inside the "if (!ret)" statement
2. In case that crypto_has_comp() returned 0, change the value of ret
to non-zero before "goto out" to indicate an error.
Signed-off-by: Piotr Sarna <p.sarna@partner.samsung.com>
Acked-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
drivers/staging/zcache/zcache-main.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/zcache/zcache-main.c b/drivers/staging/zcache/zcache-main.c
index dcceed2..81972fa 100644
--- a/drivers/staging/zcache/zcache-main.c
+++ b/drivers/staging/zcache/zcache-main.c
@@ -1811,10 +1811,12 @@ static int zcache_comp_init(void)
#else
if (*zcache_comp_name != '\0') {
ret = crypto_has_comp(zcache_comp_name, 0, 0);
- if (!ret)
+ if (!ret) {
pr_info("zcache: %s not supported\n",
zcache_comp_name);
- goto out;
+ ret = 1;
+ goto out;
+ }
}
if (!ret)
strcpy(zcache_comp_name, "lzo");
--
1.7.9.5
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH] zcache: fix "zcache=" kernel parameter
2013-07-19 14:46 [PATCH] zcache: fix "zcache=" kernel parameter Piotr Sarna
@ 2013-07-19 15:20 ` Konrad Rzeszutek Wilk
2013-07-24 10:02 ` Michal Hocko
1 sibling, 0 replies; 13+ messages in thread
From: Konrad Rzeszutek Wilk @ 2013-07-19 15:20 UTC (permalink / raw)
To: Piotr Sarna
Cc: bob.liu, gregkh, linux-kernel, devel, b.zolnierkie, Kyungmin Park
On Fri, Jul 19, 2013 at 04:46:41PM +0200, Piotr Sarna wrote:
> Commit 835f2f5 ("staging: zcache: enable zcache to be built/loaded as
> a module") introduced an incorrect handling of "zcache=" parameter.
>
> Inside zcache_comp_init() function, zcache_comp_name variable is
> checked for being empty. If not empty, the above variable is tested
> for being compatible with Crypto API. Unfortunately, after that
> function ends unconditionally (by the "goto out" directive) and returns:
> - non-zero value if verification succeeded, wrongly indicating an error
> - zero value if verification failed, falsely informing that function
> zcache_comp_init() ended properly.
>
> A solution to this problem is as following:
> 1. Move the "goto out" directive inside the "if (!ret)" statement
> 2. In case that crypto_has_comp() returned 0, change the value of ret
> to non-zero before "goto out" to indicate an error.
>
> Signed-off-by: Piotr Sarna <p.sarna@partner.samsung.com>
> Acked-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
You can also add
Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
> drivers/staging/zcache/zcache-main.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/zcache/zcache-main.c b/drivers/staging/zcache/zcache-main.c
> index dcceed2..81972fa 100644
> --- a/drivers/staging/zcache/zcache-main.c
> +++ b/drivers/staging/zcache/zcache-main.c
> @@ -1811,10 +1811,12 @@ static int zcache_comp_init(void)
> #else
> if (*zcache_comp_name != '\0') {
> ret = crypto_has_comp(zcache_comp_name, 0, 0);
> - if (!ret)
> + if (!ret) {
> pr_info("zcache: %s not supported\n",
> zcache_comp_name);
> - goto out;
> + ret = 1;
> + goto out;
> + }
> }
> if (!ret)
> strcpy(zcache_comp_name, "lzo");
> --
> 1.7.9.5
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] zcache: fix "zcache=" kernel parameter
2013-07-19 14:46 [PATCH] zcache: fix "zcache=" kernel parameter Piotr Sarna
2013-07-19 15:20 ` Konrad Rzeszutek Wilk
@ 2013-07-24 10:02 ` Michal Hocko
2013-07-24 10:04 ` Michal Hocko
1 sibling, 1 reply; 13+ messages in thread
From: Michal Hocko @ 2013-07-24 10:02 UTC (permalink / raw)
To: gregkh
Cc: Piotr Sarna, konrad.wilk, bob.liu, linux-kernel, devel,
b.zolnierkie, Kyungmin Park
I have posted a similar fix quite some time ago and I guess Greg should
already have it.
Greg?
On Fri 19-07-13 16:46:41, Piotr Sarna wrote:
> Commit 835f2f5 ("staging: zcache: enable zcache to be built/loaded as
> a module") introduced an incorrect handling of "zcache=" parameter.
>
> Inside zcache_comp_init() function, zcache_comp_name variable is
> checked for being empty. If not empty, the above variable is tested
> for being compatible with Crypto API. Unfortunately, after that
> function ends unconditionally (by the "goto out" directive) and returns:
> - non-zero value if verification succeeded, wrongly indicating an error
> - zero value if verification failed, falsely informing that function
> zcache_comp_init() ended properly.
>
> A solution to this problem is as following:
> 1. Move the "goto out" directive inside the "if (!ret)" statement
> 2. In case that crypto_has_comp() returned 0, change the value of ret
> to non-zero before "goto out" to indicate an error.
>
> Signed-off-by: Piotr Sarna <p.sarna@partner.samsung.com>
> Acked-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
> drivers/staging/zcache/zcache-main.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/zcache/zcache-main.c b/drivers/staging/zcache/zcache-main.c
> index dcceed2..81972fa 100644
> --- a/drivers/staging/zcache/zcache-main.c
> +++ b/drivers/staging/zcache/zcache-main.c
> @@ -1811,10 +1811,12 @@ static int zcache_comp_init(void)
> #else
> if (*zcache_comp_name != '\0') {
> ret = crypto_has_comp(zcache_comp_name, 0, 0);
> - if (!ret)
> + if (!ret) {
> pr_info("zcache: %s not supported\n",
> zcache_comp_name);
> - goto out;
> + ret = 1;
> + goto out;
> + }
> }
> if (!ret)
> strcpy(zcache_comp_name, "lzo");
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] zcache: fix "zcache=" kernel parameter
2013-07-24 10:02 ` Michal Hocko
@ 2013-07-24 10:04 ` Michal Hocko
2013-07-24 10:32 ` Bartlomiej Zolnierkiewicz
0 siblings, 1 reply; 13+ messages in thread
From: Michal Hocko @ 2013-07-24 10:04 UTC (permalink / raw)
To: gregkh
Cc: Piotr Sarna, konrad.wilk, bob.liu, linux-kernel, devel,
b.zolnierkie, Kyungmin Park
On Wed 24-07-13 12:02:35, Michal Hocko wrote:
> I have posted a similar fix quite some time ago and I guess Greg should
> already have it.
For reference https://lkml.org/lkml/2013/6/26/410
> Greg?
>
> On Fri 19-07-13 16:46:41, Piotr Sarna wrote:
> > Commit 835f2f5 ("staging: zcache: enable zcache to be built/loaded as
> > a module") introduced an incorrect handling of "zcache=" parameter.
> >
> > Inside zcache_comp_init() function, zcache_comp_name variable is
> > checked for being empty. If not empty, the above variable is tested
> > for being compatible with Crypto API. Unfortunately, after that
> > function ends unconditionally (by the "goto out" directive) and returns:
> > - non-zero value if verification succeeded, wrongly indicating an error
> > - zero value if verification failed, falsely informing that function
> > zcache_comp_init() ended properly.
> >
> > A solution to this problem is as following:
> > 1. Move the "goto out" directive inside the "if (!ret)" statement
> > 2. In case that crypto_has_comp() returned 0, change the value of ret
> > to non-zero before "goto out" to indicate an error.
> >
> > Signed-off-by: Piotr Sarna <p.sarna@partner.samsung.com>
> > Acked-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> > ---
> > drivers/staging/zcache/zcache-main.c | 6 ++++--
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/staging/zcache/zcache-main.c b/drivers/staging/zcache/zcache-main.c
> > index dcceed2..81972fa 100644
> > --- a/drivers/staging/zcache/zcache-main.c
> > +++ b/drivers/staging/zcache/zcache-main.c
> > @@ -1811,10 +1811,12 @@ static int zcache_comp_init(void)
> > #else
> > if (*zcache_comp_name != '\0') {
> > ret = crypto_has_comp(zcache_comp_name, 0, 0);
> > - if (!ret)
> > + if (!ret) {
> > pr_info("zcache: %s not supported\n",
> > zcache_comp_name);
> > - goto out;
> > + ret = 1;
> > + goto out;
> > + }
> > }
> > if (!ret)
> > strcpy(zcache_comp_name, "lzo");
> > --
> > 1.7.9.5
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at http://www.tux.org/lkml/
>
> --
> Michal Hocko
> SUSE Labs
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: Re: [PATCH] zcache: fix "zcache=" kernel parameter
2013-07-24 10:04 ` Michal Hocko
@ 2013-07-24 10:32 ` Bartlomiej Zolnierkiewicz
2013-07-24 11:41 ` Michal Hocko
0 siblings, 1 reply; 13+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2013-07-24 10:32 UTC (permalink / raw)
To: Michal Hocko
Cc: gregkh, Piotr Sarna, konrad.wilk, bob.liu, linux-kernel, devel,
Kyungmin Park
Hi,
On Wednesday, July 24, 2013 12:04:41 PM Michal Hocko wrote:
> On Wed 24-07-13 12:02:35, Michal Hocko wrote:
> > I have posted a similar fix quite some time ago and I guess Greg should
> > already have it.
>
> For reference https://lkml.org/lkml/2013/6/26/410
There was a reply from Greg:
https://lkml.org/lkml/2013/6/26/410
and it seems that Greg's request has not been fullfilled.
Anyway Piotr's patch seems to be more complete:
- it also fixes case in which invalid "zcache=" option is given (returns
an error value 1 while with your patch the code still erronously retuns 0)
- it prints information about compressor type being used when "zcache="
option is used (your patch skips it due to addition of "goto out_alloc")
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
> > Greg?
> >
> > On Fri 19-07-13 16:46:41, Piotr Sarna wrote:
> > > Commit 835f2f5 ("staging: zcache: enable zcache to be built/loaded as
> > > a module") introduced an incorrect handling of "zcache=" parameter.
> > >
> > > Inside zcache_comp_init() function, zcache_comp_name variable is
> > > checked for being empty. If not empty, the above variable is tested
> > > for being compatible with Crypto API. Unfortunately, after that
> > > function ends unconditionally (by the "goto out" directive) and returns:
> > > - non-zero value if verification succeeded, wrongly indicating an error
> > > - zero value if verification failed, falsely informing that function
> > > zcache_comp_init() ended properly.
> > >
> > > A solution to this problem is as following:
> > > 1. Move the "goto out" directive inside the "if (!ret)" statement
> > > 2. In case that crypto_has_comp() returned 0, change the value of ret
> > > to non-zero before "goto out" to indicate an error.
> > >
> > > Signed-off-by: Piotr Sarna <p.sarna@partner.samsung.com>
> > > Acked-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> > > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> > > ---
> > > drivers/staging/zcache/zcache-main.c | 6 ++++--
> > > 1 file changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/staging/zcache/zcache-main.c b/drivers/staging/zcache/zcache-main.c
> > > index dcceed2..81972fa 100644
> > > --- a/drivers/staging/zcache/zcache-main.c
> > > +++ b/drivers/staging/zcache/zcache-main.c
> > > @@ -1811,10 +1811,12 @@ static int zcache_comp_init(void)
> > > #else
> > > if (*zcache_comp_name != '\0') {
> > > ret = crypto_has_comp(zcache_comp_name, 0, 0);
> > > - if (!ret)
> > > + if (!ret) {
> > > pr_info("zcache: %s not supported\n",
> > > zcache_comp_name);
> > > - goto out;
> > > + ret = 1;
> > > + goto out;
> > > + }
> > > }
> > > if (!ret)
> > > strcpy(zcache_comp_name, "lzo");
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: Re: [PATCH] zcache: fix "zcache=" kernel parameter
2013-07-24 10:32 ` Bartlomiej Zolnierkiewicz
@ 2013-07-24 11:41 ` Michal Hocko
2013-07-24 11:54 ` Bartlomiej Zolnierkiewicz
0 siblings, 1 reply; 13+ messages in thread
From: Michal Hocko @ 2013-07-24 11:41 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz
Cc: gregkh, Piotr Sarna, konrad.wilk, bob.liu, linux-kernel, devel,
Kyungmin Park
On Wed 24-07-13 12:32:32, Bartlomiej Zolnierkiewicz wrote:
>
> Hi,
>
> On Wednesday, July 24, 2013 12:04:41 PM Michal Hocko wrote:
> > On Wed 24-07-13 12:02:35, Michal Hocko wrote:
> > > I have posted a similar fix quite some time ago and I guess Greg should
> > > already have it.
> >
> > For reference https://lkml.org/lkml/2013/6/26/410
>
> There was a reply from Greg:
>
> https://lkml.org/lkml/2013/6/26/410
I guess you meant https://lkml.org/lkml/2013/6/26/569
> and it seems that Greg's request has not been fullfilled.
Bob Liu has resent the patch (I cannot find the email in the archive).
> Anyway Piotr's patch seems to be more complete:
> - it also fixes case in which invalid "zcache=" option is given (returns
> an error value 1 while with your patch the code still erronously retuns 0)
> - it prints information about compressor type being used when "zcache="
> option is used (your patch skips it due to addition of "goto out_alloc")
I do not care which one will make it I just pointed out that there is
another patch dealing with the same and it is a question how far that
one went.
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] zcache: fix "zcache=" kernel parameter
2013-07-24 11:41 ` Michal Hocko
@ 2013-07-24 11:54 ` Bartlomiej Zolnierkiewicz
2013-07-24 15:04 ` Greg KH
2013-07-24 15:06 ` Greg KH
0 siblings, 2 replies; 13+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2013-07-24 11:54 UTC (permalink / raw)
To: Michal Hocko
Cc: gregkh, Piotr Sarna, konrad.wilk, bob.liu, linux-kernel, devel,
Kyungmin Park
On Wednesday, July 24, 2013 01:41:57 PM Michal Hocko wrote:
> On Wed 24-07-13 12:32:32, Bartlomiej Zolnierkiewicz wrote:
> >
> > Hi,
> >
> > On Wednesday, July 24, 2013 12:04:41 PM Michal Hocko wrote:
> > > On Wed 24-07-13 12:02:35, Michal Hocko wrote:
> > > > I have posted a similar fix quite some time ago and I guess Greg should
> > > > already have it.
> > >
> > > For reference https://lkml.org/lkml/2013/6/26/410
> >
> > There was a reply from Greg:
> >
> > https://lkml.org/lkml/2013/6/26/410
>
> I guess you meant https://lkml.org/lkml/2013/6/26/569
Uh, yes.
> > and it seems that Greg's request has not been fullfilled.
>
> Bob Liu has resent the patch (I cannot find the email in the archive).
Indeed, I've found it here:
http://comments.gmane.org/gmane.linux.kernel.mm/102484
> > Anyway Piotr's patch seems to be more complete:
> > - it also fixes case in which invalid "zcache=" option is given (returns
> > an error value 1 while with your patch the code still erronously retuns 0)
> > - it prints information about compressor type being used when "zcache="
> > option is used (your patch skips it due to addition of "goto out_alloc")
>
> I do not care which one will make it I just pointed out that there is
> another patch dealing with the same and it is a question how far that
> one went.
OK.
Greg, could you please pick Piotr's patch?
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] zcache: fix "zcache=" kernel parameter
2013-07-24 11:54 ` Bartlomiej Zolnierkiewicz
@ 2013-07-24 15:04 ` Greg KH
2013-07-24 15:06 ` Greg KH
1 sibling, 0 replies; 13+ messages in thread
From: Greg KH @ 2013-07-24 15:04 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz
Cc: Michal Hocko, devel, konrad.wilk, Piotr Sarna, linux-kernel,
bob.liu, Kyungmin Park
On Wed, Jul 24, 2013 at 01:54:56PM +0200, Bartlomiej Zolnierkiewicz wrote:
>
> On Wednesday, July 24, 2013 01:41:57 PM Michal Hocko wrote:
> > On Wed 24-07-13 12:32:32, Bartlomiej Zolnierkiewicz wrote:
> > >
> > > Hi,
> > >
> > > On Wednesday, July 24, 2013 12:04:41 PM Michal Hocko wrote:
> > > > On Wed 24-07-13 12:02:35, Michal Hocko wrote:
> > > > > I have posted a similar fix quite some time ago and I guess Greg should
> > > > > already have it.
> > > >
> > > > For reference https://lkml.org/lkml/2013/6/26/410
> > >
> > > There was a reply from Greg:
> > >
> > > https://lkml.org/lkml/2013/6/26/410
> >
> > I guess you meant https://lkml.org/lkml/2013/6/26/569
>
> Uh, yes.
>
> > > and it seems that Greg's request has not been fullfilled.
> >
> > Bob Liu has resent the patch (I cannot find the email in the archive).
>
> Indeed, I've found it here:
>
> http://comments.gmane.org/gmane.linux.kernel.mm/102484
>
> > > Anyway Piotr's patch seems to be more complete:
> > > - it also fixes case in which invalid "zcache=" option is given (returns
> > > an error value 1 while with your patch the code still erronously retuns 0)
> > > - it prints information about compressor type being used when "zcache="
> > > option is used (your patch skips it due to addition of "goto out_alloc")
> >
> > I do not care which one will make it I just pointed out that there is
> > another patch dealing with the same and it is a question how far that
> > one went.
>
> OK.
>
> Greg, could you please pick Piotr's patch?
I'll get to it. As no one labled it with "staging" at the front of the
patch, it missed my sweep of all needed staging patches, and fell into
the "I'll get to them soon" portion of my week.
Also, as this is staging code, it really isn't that important, otherwise
people would have worked harder to get it out of that portion of the
tree :)
{hint...}
Don't worry, it's not lost,
greg k-h
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] zcache: fix "zcache=" kernel parameter
2013-07-24 11:54 ` Bartlomiej Zolnierkiewicz
2013-07-24 15:04 ` Greg KH
@ 2013-07-24 15:06 ` Greg KH
2013-07-24 15:39 ` Bartlomiej Zolnierkiewicz
1 sibling, 1 reply; 13+ messages in thread
From: Greg KH @ 2013-07-24 15:06 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz
Cc: Michal Hocko, devel, konrad.wilk, Piotr Sarna, linux-kernel,
bob.liu, Kyungmin Park
On Wed, Jul 24, 2013 at 01:54:56PM +0200, Bartlomiej Zolnierkiewicz wrote:
>
> On Wednesday, July 24, 2013 01:41:57 PM Michal Hocko wrote:
> > On Wed 24-07-13 12:32:32, Bartlomiej Zolnierkiewicz wrote:
> > >
> > > Hi,
> > >
> > > On Wednesday, July 24, 2013 12:04:41 PM Michal Hocko wrote:
> > > > On Wed 24-07-13 12:02:35, Michal Hocko wrote:
> > > > > I have posted a similar fix quite some time ago and I guess Greg should
> > > > > already have it.
> > > >
> > > > For reference https://lkml.org/lkml/2013/6/26/410
> > >
> > > There was a reply from Greg:
> > >
> > > https://lkml.org/lkml/2013/6/26/410
> >
> > I guess you meant https://lkml.org/lkml/2013/6/26/569
>
> Uh, yes.
>
> > > and it seems that Greg's request has not been fullfilled.
> >
> > Bob Liu has resent the patch (I cannot find the email in the archive).
>
> Indeed, I've found it here:
>
> http://comments.gmane.org/gmane.linux.kernel.mm/102484
>
> > > Anyway Piotr's patch seems to be more complete:
> > > - it also fixes case in which invalid "zcache=" option is given (returns
> > > an error value 1 while with your patch the code still erronously retuns 0)
> > > - it prints information about compressor type being used when "zcache="
> > > option is used (your patch skips it due to addition of "goto out_alloc")
> >
> > I do not care which one will make it I just pointed out that there is
> > another patch dealing with the same and it is a question how far that
> > one went.
>
> OK.
>
> Greg, could you please pick Piotr's patch?
Wait, I'm going to take the first one I got, from Michal, as that's the
original author here, not Piotr...
confused, and really grumpy about the whole zcache mess...
greg k-h
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] zcache: fix "zcache=" kernel parameter
2013-07-24 15:06 ` Greg KH
@ 2013-07-24 15:39 ` Bartlomiej Zolnierkiewicz
2013-07-24 15:50 ` Bartlomiej Zolnierkiewicz
0 siblings, 1 reply; 13+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2013-07-24 15:39 UTC (permalink / raw)
To: Greg KH
Cc: Michal Hocko, devel, konrad.wilk, Piotr Sarna, linux-kernel,
bob.liu, Kyungmin Park, Cristian Rodríguez
On Wednesday, July 24, 2013 08:06:27 AM Greg KH wrote:
> On Wed, Jul 24, 2013 at 01:54:56PM +0200, Bartlomiej Zolnierkiewicz wrote:
> >
> > On Wednesday, July 24, 2013 01:41:57 PM Michal Hocko wrote:
> > > On Wed 24-07-13 12:32:32, Bartlomiej Zolnierkiewicz wrote:
> > > >
> > > > Hi,
> > > >
> > > > On Wednesday, July 24, 2013 12:04:41 PM Michal Hocko wrote:
> > > > > On Wed 24-07-13 12:02:35, Michal Hocko wrote:
> > > > > > I have posted a similar fix quite some time ago and I guess Greg should
> > > > > > already have it.
> > > > >
> > > > > For reference https://lkml.org/lkml/2013/6/26/410
> > > >
> > > > There was a reply from Greg:
> > > >
> > > > https://lkml.org/lkml/2013/6/26/410
> > >
> > > I guess you meant https://lkml.org/lkml/2013/6/26/569
> >
> > Uh, yes.
> >
> > > > and it seems that Greg's request has not been fullfilled.
> > >
> > > Bob Liu has resent the patch (I cannot find the email in the archive).
> >
> > Indeed, I've found it here:
> >
> > http://comments.gmane.org/gmane.linux.kernel.mm/102484
> >
> > > > Anyway Piotr's patch seems to be more complete:
> > > > - it also fixes case in which invalid "zcache=" option is given (returns
> > > > an error value 1 while with your patch the code still erronously retuns 0)
> > > > - it prints information about compressor type being used when "zcache="
> > > > option is used (your patch skips it due to addition of "goto out_alloc")
> > >
> > > I do not care which one will make it I just pointed out that there is
> > > another patch dealing with the same and it is a question how far that
> > > one went.
> >
> > OK.
> >
> > Greg, could you please pick Piotr's patch?
>
> Wait, I'm going to take the first one I got, from Michal, as that's the
> original author here, not Piotr...
>
> confused, and really grumpy about the whole zcache mess...
Just get the second one (from Piotr) as it is more complete (it fixes
also problem with invalid data being given to "zcache=" option or chosen
compressor not being supported by Crypto API).
I feel a bit sad that this newer patch makes Michal's one obsolete but
I don't have a good idea how to resolve it in other way while sticking to
"chose the best technical solution" principle. :(
Oh, wait. I updated patch (attached below) to give Michal & Cristian some
credit (also added Konrad's Acked-by while at it). I hope that this is
an acceptable solution.
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
From: Piotr Sarna <p.sarna@partner.samsung.com>
Subject: [PATCH] zcache: fix "zcache=" kernel parameter
Commit 835f2f5 ("staging: zcache: enable zcache to be built/loaded as
a module") introduced an incorrect handling of "zcache=" parameter.
Inside zcache_comp_init() function, zcache_comp_name variable is
checked for being empty. If not empty, the above variable is tested
for being compatible with Crypto API. Unfortunately, after that
function ends unconditionally (by the "goto out" directive) and returns:
- non-zero value if verification succeeded, wrongly indicating an error
- zero value if verification failed, falsely informing that function
zcache_comp_init() ended properly.
A solution to this problem is as following:
1. Move the "goto out" directive inside the "if (!ret)" statement
2. In case that crypto_has_comp() returned 0, change the value of ret
to non-zero before "goto out" to indicate an error.
This patch replaces an earlier one from Michal Hocko (based on report
from Cristian Rodríguez):
http://comments.gmane.org/gmane.linux.kernel.mm/102484
It also addressed the same issue but didn't fix the zcache_comp_init()
for case when the compressor data passed to "zcache=" option was invalid
or unsupported.
Signed-off-by: Piotr Sarna <p.sarna@partner.samsung.com>
[bzolnier: updated patch description]
Acked-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Cristian Rodríguez <crrodriguez@opensuse.org>
Cc: Michal Hocko <mhocko@suse.cz>
Cc: Bob Liu <bob.liu <at> oracle.com>
---
drivers/staging/zcache/zcache-main.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/zcache/zcache-main.c b/drivers/staging/zcache/zcache-main.c
index dcceed2..81972fa 100644
--- a/drivers/staging/zcache/zcache-main.c
+++ b/drivers/staging/zcache/zcache-main.c
@@ -1811,10 +1811,12 @@ static int zcache_comp_init(void)
#else
if (*zcache_comp_name != '\0') {
ret = crypto_has_comp(zcache_comp_name, 0, 0);
- if (!ret)
+ if (!ret) {
pr_info("zcache: %s not supported\n",
zcache_comp_name);
- goto out;
+ ret = 1;
+ goto out;
+ }
}
if (!ret)
strcpy(zcache_comp_name, "lzo");
--
1.7.9.5
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH] zcache: fix "zcache=" kernel parameter
2013-07-24 15:39 ` Bartlomiej Zolnierkiewicz
@ 2013-07-24 15:50 ` Bartlomiej Zolnierkiewicz
2013-07-24 16:48 ` Greg KH
0 siblings, 1 reply; 13+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2013-07-24 15:50 UTC (permalink / raw)
To: Greg KH
Cc: Michal Hocko, devel, konrad.wilk, Piotr Sarna, linux-kernel,
bob.liu, Kyungmin Park, Cristian Rodríguez
On Wednesday, July 24, 2013 05:39:20 PM Bartlomiej Zolnierkiewicz wrote:
>
> On Wednesday, July 24, 2013 08:06:27 AM Greg KH wrote:
> > On Wed, Jul 24, 2013 at 01:54:56PM +0200, Bartlomiej Zolnierkiewicz wrote:
> > >
> > > On Wednesday, July 24, 2013 01:41:57 PM Michal Hocko wrote:
> > > > On Wed 24-07-13 12:32:32, Bartlomiej Zolnierkiewicz wrote:
> > > > >
> > > > > Hi,
> > > > >
> > > > > On Wednesday, July 24, 2013 12:04:41 PM Michal Hocko wrote:
> > > > > > On Wed 24-07-13 12:02:35, Michal Hocko wrote:
> > > > > > > I have posted a similar fix quite some time ago and I guess Greg should
> > > > > > > already have it.
> > > > > >
> > > > > > For reference https://lkml.org/lkml/2013/6/26/410
> > > > >
> > > > > There was a reply from Greg:
> > > > >
> > > > > https://lkml.org/lkml/2013/6/26/410
> > > >
> > > > I guess you meant https://lkml.org/lkml/2013/6/26/569
> > >
> > > Uh, yes.
> > >
> > > > > and it seems that Greg's request has not been fullfilled.
> > > >
> > > > Bob Liu has resent the patch (I cannot find the email in the archive).
> > >
> > > Indeed, I've found it here:
> > >
> > > http://comments.gmane.org/gmane.linux.kernel.mm/102484
> > >
> > > > > Anyway Piotr's patch seems to be more complete:
> > > > > - it also fixes case in which invalid "zcache=" option is given (returns
> > > > > an error value 1 while with your patch the code still erronously retuns 0)
> > > > > - it prints information about compressor type being used when "zcache="
> > > > > option is used (your patch skips it due to addition of "goto out_alloc")
> > > >
> > > > I do not care which one will make it I just pointed out that there is
> > > > another patch dealing with the same and it is a question how far that
> > > > one went.
> > >
> > > OK.
> > >
> > > Greg, could you please pick Piotr's patch?
> >
> > Wait, I'm going to take the first one I got, from Michal, as that's the
> > original author here, not Piotr...
> >
> > confused, and really grumpy about the whole zcache mess...
>
> Just get the second one (from Piotr) as it is more complete (it fixes
> also problem with invalid data being given to "zcache=" option or chosen
> compressor not being supported by Crypto API).
>
> I feel a bit sad that this newer patch makes Michal's one obsolete but
> I don't have a good idea how to resolve it in other way while sticking to
> "chose the best technical solution" principle. :(
>
> Oh, wait. I updated patch (attached below) to give Michal & Cristian some
> credit (also added Konrad's Acked-by while at it). I hope that this is
> an acceptable solution.
Michal/Cristian, are you fine with that? Could you please Ack this patch?
> Best regards,
> --
> Bartlomiej Zolnierkiewicz
> Samsung R&D Institute Poland
> Samsung Electronics
>
>
> From: Piotr Sarna <p.sarna@partner.samsung.com>
> Subject: [PATCH] zcache: fix "zcache=" kernel parameter
...
This time with fixed Bob's email address and better link to an old
patch. Sorry for that.
From: Piotr Sarna <p.sarna@partner.samsung.com>
Subject: [PATCH] zcache: fix "zcache=" kernel parameter
Commit 835f2f5 ("staging: zcache: enable zcache to be built/loaded as
a module") introduced an incorrect handling of "zcache=" parameter.
Inside zcache_comp_init() function, zcache_comp_name variable is
checked for being empty. If not empty, the above variable is tested
for being compatible with Crypto API. Unfortunately, after that
function ends unconditionally (by the "goto out" directive) and returns:
- non-zero value if verification succeeded, wrongly indicating an error
- zero value if verification failed, falsely informing that function
zcache_comp_init() ended properly.
A solution to this problem is as following:
1. Move the "goto out" directive inside the "if (!ret)" statement
2. In case that crypto_has_comp() returned 0, change the value of ret
to non-zero before "goto out" to indicate an error.
This patch replaces an earlier one from Michal Hocko (based on report
from Cristian Rodríguez):
http://permalink.gmane.org/gmane.linux.kernel.mm/102484
It also addressed the same issue but didn't fix the zcache_comp_init()
for case when the compressor data passed to "zcache=" option was invalid
or unsupported.
Signed-off-by: Piotr Sarna <p.sarna@partner.samsung.com>
[bzolnier: updated patch description]
Acked-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Cristian Rodríguez <crrodriguez@opensuse.org>
Cc: Michal Hocko <mhocko@suse.cz>
Cc: Bob Liu <bob.liu@oracle.com>
---
drivers/staging/zcache/zcache-main.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/zcache/zcache-main.c b/drivers/staging/zcache/zcache-main.c
index dcceed2..81972fa 100644
--- a/drivers/staging/zcache/zcache-main.c
+++ b/drivers/staging/zcache/zcache-main.c
@@ -1811,10 +1811,12 @@ static int zcache_comp_init(void)
#else
if (*zcache_comp_name != '\0') {
ret = crypto_has_comp(zcache_comp_name, 0, 0);
- if (!ret)
+ if (!ret) {
pr_info("zcache: %s not supported\n",
zcache_comp_name);
- goto out;
+ ret = 1;
+ goto out;
+ }
}
if (!ret)
strcpy(zcache_comp_name, "lzo");
--
1.7.9.5
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH] zcache: fix "zcache=" kernel parameter
2013-07-24 15:50 ` Bartlomiej Zolnierkiewicz
@ 2013-07-24 16:48 ` Greg KH
0 siblings, 0 replies; 13+ messages in thread
From: Greg KH @ 2013-07-24 16:48 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz
Cc: devel, konrad.wilk, Cristian Rodríguez, Piotr Sarna,
linux-kernel, Michal Hocko, bob.liu, Kyungmin Park
On Wed, Jul 24, 2013 at 05:50:29PM +0200, Bartlomiej Zolnierkiewicz wrote:
>
> On Wednesday, July 24, 2013 05:39:20 PM Bartlomiej Zolnierkiewicz wrote:
> >
> > On Wednesday, July 24, 2013 08:06:27 AM Greg KH wrote:
> > > On Wed, Jul 24, 2013 at 01:54:56PM +0200, Bartlomiej Zolnierkiewicz wrote:
> > > >
> > > > On Wednesday, July 24, 2013 01:41:57 PM Michal Hocko wrote:
> > > > > On Wed 24-07-13 12:32:32, Bartlomiej Zolnierkiewicz wrote:
> > > > > >
> > > > > > Hi,
> > > > > >
> > > > > > On Wednesday, July 24, 2013 12:04:41 PM Michal Hocko wrote:
> > > > > > > On Wed 24-07-13 12:02:35, Michal Hocko wrote:
> > > > > > > > I have posted a similar fix quite some time ago and I guess Greg should
> > > > > > > > already have it.
> > > > > > >
> > > > > > > For reference https://lkml.org/lkml/2013/6/26/410
> > > > > >
> > > > > > There was a reply from Greg:
> > > > > >
> > > > > > https://lkml.org/lkml/2013/6/26/410
> > > > >
> > > > > I guess you meant https://lkml.org/lkml/2013/6/26/569
> > > >
> > > > Uh, yes.
> > > >
> > > > > > and it seems that Greg's request has not been fullfilled.
> > > > >
> > > > > Bob Liu has resent the patch (I cannot find the email in the archive).
> > > >
> > > > Indeed, I've found it here:
> > > >
> > > > http://comments.gmane.org/gmane.linux.kernel.mm/102484
> > > >
> > > > > > Anyway Piotr's patch seems to be more complete:
> > > > > > - it also fixes case in which invalid "zcache=" option is given (returns
> > > > > > an error value 1 while with your patch the code still erronously retuns 0)
> > > > > > - it prints information about compressor type being used when "zcache="
> > > > > > option is used (your patch skips it due to addition of "goto out_alloc")
> > > > >
> > > > > I do not care which one will make it I just pointed out that there is
> > > > > another patch dealing with the same and it is a question how far that
> > > > > one went.
> > > >
> > > > OK.
> > > >
> > > > Greg, could you please pick Piotr's patch?
> > >
> > > Wait, I'm going to take the first one I got, from Michal, as that's the
> > > original author here, not Piotr...
> > >
> > > confused, and really grumpy about the whole zcache mess...
> >
> > Just get the second one (from Piotr) as it is more complete (it fixes
> > also problem with invalid data being given to "zcache=" option or chosen
> > compressor not being supported by Crypto API).
> >
> > I feel a bit sad that this newer patch makes Michal's one obsolete but
> > I don't have a good idea how to resolve it in other way while sticking to
> > "chose the best technical solution" principle. :(
> >
> > Oh, wait. I updated patch (attached below) to give Michal & Cristian some
> > credit (also added Konrad's Acked-by while at it). I hope that this is
> > an acceptable solution.
>
> Michal/Cristian, are you fine with that? Could you please Ack this patch?
>
> > Best regards,
> > --
> > Bartlomiej Zolnierkiewicz
> > Samsung R&D Institute Poland
> > Samsung Electronics
> >
> >
> > From: Piotr Sarna <p.sarna@partner.samsung.com>
> > Subject: [PATCH] zcache: fix "zcache=" kernel parameter
>
> ...
>
> This time with fixed Bob's email address and better link to an old
> patch. Sorry for that.
>
> From: Piotr Sarna <p.sarna@partner.samsung.com>
> Subject: [PATCH] zcache: fix "zcache=" kernel parameter
Now, can you resend it in a format that doesn't require me to hand-edit
the patch? As I deal with thousands of patches every kernel release, I
can't clean up the mess by hand and expect to stay sane...
thanks,
greg k-h
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH] zcache: fix "zcache=" kernel parameter
@ 2013-07-18 11:09 Piotr Sarna
0 siblings, 0 replies; 13+ messages in thread
From: Piotr Sarna @ 2013-07-18 11:09 UTC (permalink / raw)
To: dan.magenheimer
Cc: linux-kernel, devel, b.zolnierkie, Piotr Sarna, Kyungmin Park
Commit 835f2f5 ("staging: zcache: enable zcache to be built/loaded as
a module") introduced an incorrect handling of "zcache=" parameter.
Inside zcache_comp_init() function, zcache_comp_name variable is
checked for being empty. If not empty, the above variable is tested
for being compatible with Crypto API. Unfortunately, after that
function ends unconditionally (by the "goto out" directive) and returns:
- non-zero value if verification succeeded, wrongly indicating an error
- zero value if verification failed, falsely informing that function
zcache_comp_init() ended properly.
A solution to this problem is as following:
1. Move the "goto out" directive inside the "if (!ret)" statement
2. In case that crypto_has_comp() returned 0, change the value of ret
to non-zero before "goto out" to indicate an error.
Signed-off-by: Piotr Sarna <p.sarna@partner.samsung.com>
Acked-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
drivers/staging/zcache/zcache-main.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/zcache/zcache-main.c b/drivers/staging/zcache/zcache-main.c
index dcceed2..81972fa 100644
--- a/drivers/staging/zcache/zcache-main.c
+++ b/drivers/staging/zcache/zcache-main.c
@@ -1811,10 +1811,12 @@ static int zcache_comp_init(void)
#else
if (*zcache_comp_name != '\0') {
ret = crypto_has_comp(zcache_comp_name, 0, 0);
- if (!ret)
+ if (!ret) {
pr_info("zcache: %s not supported\n",
zcache_comp_name);
- goto out;
+ ret = 1;
+ goto out;
+ }
}
if (!ret)
strcpy(zcache_comp_name, "lzo");
--
1.7.9.5
^ permalink raw reply related [flat|nested] 13+ messages in thread
end of thread, other threads:[~2013-07-24 16:48 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-07-19 14:46 [PATCH] zcache: fix "zcache=" kernel parameter Piotr Sarna
2013-07-19 15:20 ` Konrad Rzeszutek Wilk
2013-07-24 10:02 ` Michal Hocko
2013-07-24 10:04 ` Michal Hocko
2013-07-24 10:32 ` Bartlomiej Zolnierkiewicz
2013-07-24 11:41 ` Michal Hocko
2013-07-24 11:54 ` Bartlomiej Zolnierkiewicz
2013-07-24 15:04 ` Greg KH
2013-07-24 15:06 ` Greg KH
2013-07-24 15:39 ` Bartlomiej Zolnierkiewicz
2013-07-24 15:50 ` Bartlomiej Zolnierkiewicz
2013-07-24 16:48 ` Greg KH
-- strict thread matches above, loose matches on Subject: below --
2013-07-18 11:09 Piotr Sarna
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).