From: Mike Snitzer <snitzer@redhat.com>
To: Shuah Khan <skhan@linuxfoundation.org>
Cc: dm-devel@redhat.com, linux-kernel@vger.kernel.org, agk@redhat.com
Subject: Re: [dm-devel] dm: change dm_get_target_type() to check for module load error
Date: Wed, 6 Oct 2021 16:35:05 -0400 [thread overview]
Message-ID: <YV4IecLg56NpzkYx@redhat.com> (raw)
In-Reply-To: <20211004200641.378496-1-skhan@linuxfoundation.org>
On Mon, Oct 04 2021 at 4:06P -0400,
Shuah Khan <skhan@linuxfoundation.org> wrote:
> dm_get_target_type() doesn't check error return from request_module().
> Change to check for error and return NULL instead of trying to get
> target type again which would fail.
>
> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
> ---
> drivers/md/dm-target.c | 11 ++++++++---
> 1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/md/dm-target.c b/drivers/md/dm-target.c
> index 64dd0b34fcf4..0789e9f91d3a 100644
> --- a/drivers/md/dm-target.c
> +++ b/drivers/md/dm-target.c
> @@ -41,17 +41,22 @@ static struct target_type *get_target_type(const char *name)
> return tt;
> }
>
> -static void load_module(const char *name)
> +static int load_module(const char *name)
> {
> - request_module("dm-%s", name);
> + return request_module("dm-%s", name);
> }
>
> struct target_type *dm_get_target_type(const char *name)
> {
> struct target_type *tt = get_target_type(name);
> + int ret;
>
> if (!tt) {
> - load_module(name);
> + ret = load_module(name);
> + if (ret < 0) {
> + pr_err("Module %s load failed %d\n", name, ret);
> + return NULL;
> + }
> tt = get_target_type(name);
> }
>
> --
> 2.30.2
>
While I appreciate your intent, the reality is that multiple targets
may be made available in a given module. And so loading one dm module
may bring in access to N targets. There isn't a rigid 1:1 mapping of
target modules to names. And there may not even be a loadable module
that has the name dm-${name} -- but that doesn't mean the target_type
won;t have been loaded into DM for it to access.
So all said, your patch is bogus and would break DM and user
experience:
Nacked-by: Mike Snitzer <snitzer@redhat.com>
But thanks for raising your concern.
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
WARNING: multiple messages have this Message-ID (diff)
From: Mike Snitzer <snitzer@redhat.com>
To: Shuah Khan <skhan@linuxfoundation.org>
Cc: agk@redhat.com, dm-devel@redhat.com, linux-kernel@vger.kernel.org
Subject: Re: dm: change dm_get_target_type() to check for module load error
Date: Wed, 6 Oct 2021 16:35:05 -0400 [thread overview]
Message-ID: <YV4IecLg56NpzkYx@redhat.com> (raw)
In-Reply-To: <20211004200641.378496-1-skhan@linuxfoundation.org>
On Mon, Oct 04 2021 at 4:06P -0400,
Shuah Khan <skhan@linuxfoundation.org> wrote:
> dm_get_target_type() doesn't check error return from request_module().
> Change to check for error and return NULL instead of trying to get
> target type again which would fail.
>
> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
> ---
> drivers/md/dm-target.c | 11 ++++++++---
> 1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/md/dm-target.c b/drivers/md/dm-target.c
> index 64dd0b34fcf4..0789e9f91d3a 100644
> --- a/drivers/md/dm-target.c
> +++ b/drivers/md/dm-target.c
> @@ -41,17 +41,22 @@ static struct target_type *get_target_type(const char *name)
> return tt;
> }
>
> -static void load_module(const char *name)
> +static int load_module(const char *name)
> {
> - request_module("dm-%s", name);
> + return request_module("dm-%s", name);
> }
>
> struct target_type *dm_get_target_type(const char *name)
> {
> struct target_type *tt = get_target_type(name);
> + int ret;
>
> if (!tt) {
> - load_module(name);
> + ret = load_module(name);
> + if (ret < 0) {
> + pr_err("Module %s load failed %d\n", name, ret);
> + return NULL;
> + }
> tt = get_target_type(name);
> }
>
> --
> 2.30.2
>
While I appreciate your intent, the reality is that multiple targets
may be made available in a given module. And so loading one dm module
may bring in access to N targets. There isn't a rigid 1:1 mapping of
target modules to names. And there may not even be a loadable module
that has the name dm-${name} -- but that doesn't mean the target_type
won;t have been loaded into DM for it to access.
So all said, your patch is bogus and would break DM and user
experience:
Nacked-by: Mike Snitzer <snitzer@redhat.com>
But thanks for raising your concern.
next prev parent reply other threads:[~2021-10-06 20:47 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-10-04 20:06 [dm-devel] [PATCH] dm: change dm_get_target_type() to check for module load error Shuah Khan
2021-10-04 20:06 ` Shuah Khan
2021-10-06 20:35 ` Mike Snitzer [this message]
2021-10-06 20:35 ` Mike Snitzer
2021-10-06 20:55 ` [dm-devel] " Shuah Khan
2021-10-06 20:55 ` Shuah Khan
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YV4IecLg56NpzkYx@redhat.com \
--to=snitzer@redhat.com \
--cc=agk@redhat.com \
--cc=dm-devel@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=skhan@linuxfoundation.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.