[PATCH 0/2] Fix auto resize code

[PATCH 0/2] Fix auto resize code

[Benjamin Marzinski]

Fix to bugs in my auto-resize feature.

Benjamin Marzinski (2):
  multipathd: fix null pointer dereference in uev_update_path
  multipathd: fix auto-resize configuration

 libmultipath/defaults.h | 1 -
 libmultipath/structs.h  | 3 +--
 multipathd/main.c       | 2 +-
 3 files changed, 2 insertions(+), 4 deletions(-)

-- 
2.43.0

[PATCH 1/2] multipathd: fix null pointer dereference in uev_update_path

[Benjamin Marzinski]

The Auto-resize code added a check that deferences pp->mpp without
checking that it's non-NULL. Fix it.

Fixes: 981b83ad1 ("multipathd: Add auto_resize config option")
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipathd/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 74f6cd92..fbc3f8da 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -1630,7 +1630,7 @@ uev_update_path (struct uevent *uev, struct vectors * vecs)
 				}
 			}
 		}
-		if (auto_resize != AUTO_RESIZE_NEVER &&
+		if (auto_resize != AUTO_RESIZE_NEVER && mpp &&
 		    !mpp->wait_for_udev) {
 			struct pathgroup *pgp;
 			struct path *pp2;
-- 
2.43.0

[PATCH 2/2] multipathd: fix auto-resize configuration

[Benjamin Marzinski]

The code acted like AUTO_RESIZE_UNDEFINED didn't exist, but since
conf->auto_resize was never set to AUTO_RESIZE_NEVER, the default was in
fact AUTO_RESIZE_UNDEFINED, which ended up getting treated like
AUTO_RESIZE_GROW_SHRINK. Remove AUTO_RESIZE_UNDEFINED, so
AUTO_RESIZE_NEVER really is the default.

Fixes: 981b83ad1 ("multipathd: Add auto_resize config option")
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/defaults.h | 1 -
 libmultipath/structs.h  | 3 +--
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/libmultipath/defaults.h b/libmultipath/defaults.h
index 64b633f2..d01f9712 100644
--- a/libmultipath/defaults.h
+++ b/libmultipath/defaults.h
@@ -56,7 +56,6 @@
 #define DEFAULT_UNKNOWN_FIND_MULTIPATHS_TIMEOUT 1
 #define DEFAULT_ALL_TG_PT ALL_TG_PT_OFF
 #define DEFAULT_RECHECK_WWID RECHECK_WWID_OFF
-#define DEFAULT_AUTO_RESIZE AUTO_RESIZE_NEVER
 /* Enable no foreign libraries by default */
 #define DEFAULT_ENABLE_FOREIGN "NONE"
 
diff --git a/libmultipath/structs.h b/libmultipath/structs.h
index a1aac1b4..f133e78f 100644
--- a/libmultipath/structs.h
+++ b/libmultipath/structs.h
@@ -180,8 +180,7 @@ enum queue_mode_states {
 };
 
 enum auto_resize_state {
-	AUTO_RESIZE_UNDEF = 0,
-	AUTO_RESIZE_NEVER,
+	AUTO_RESIZE_NEVER = 0,
 	AUTO_RESIZE_GROW_ONLY,
 	AUTO_RESIZE_GROW_SHRINK,
 };
-- 
2.43.0

Re: [PATCH 2/2] multipathd: fix auto-resize configuration

[Martin Wilck]

On Tue, 2024-01-16 at 02:05 -0500, Benjamin Marzinski wrote:
> The code acted like AUTO_RESIZE_UNDEFINED didn't exist, but since
> conf->auto_resize was never set to AUTO_RESIZE_NEVER, the default was
> in
> fact AUTO_RESIZE_UNDEFINED, which ended up getting treated like
> AUTO_RESIZE_GROW_SHRINK. Remove AUTO_RESIZE_UNDEFINED, so
> AUTO_RESIZE_NEVER really is the default.
> 
> Fixes: 981b83ad1 ("multipathd: Add auto_resize config option")
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>

The analysis is correct, but the fix differs from what we did for other
options. We won't be able to cleanly change the default later on with
this patch applied.

I would prefer to keep AUTO_RESIZE_UNDEF and DEFAULT_AUTO_RESIZE and
just change the condition in uev_update_path() from "auto_resize !=
AUTO_RESIZE_NEVER" to "(auto_resize == AUTO_RESIZE_GROW_ONLY ||
auto_resize == AUTO_RESIZE_GROW_SHRINK)", unless you have strong
reasons not to do so.

Regards,
Martin

Re: [PATCH 2/2] multipathd: fix auto-resize configuration

[Martin Wilck]

On Tue, 2024-01-16 at 12:23 +0100, Martin Wilck wrote:
> 
> I would prefer to keep AUTO_RESIZE_UNDEF and DEFAULT_AUTO_RESIZE and
> just change the condition in uev_update_path() from "auto_resize !=
> AUTO_RESIZE_NEVER" to "(auto_resize == AUTO_RESIZE_GROW_ONLY ||
> auto_resize == AUTO_RESIZE_GROW_SHRINK)", unless you have strong
> reasons not to do so.

... and if we do it this way, conf->auto_resize should be initialized
to DEFAULT_AUTO_RESIZE in _init_config().


Martin

Re: [PATCH 2/2] multipathd: fix auto-resize configuration

[Benjamin Marzinski]

On Tue, Jan 16, 2024 at 12:23:33PM +0100, Martin Wilck wrote:
> On Tue, 2024-01-16 at 02:05 -0500, Benjamin Marzinski wrote:
> > The code acted like AUTO_RESIZE_UNDEFINED didn't exist, but since
> > conf->auto_resize was never set to AUTO_RESIZE_NEVER, the default was
> > in
> > fact AUTO_RESIZE_UNDEFINED, which ended up getting treated like
> > AUTO_RESIZE_GROW_SHRINK. Remove AUTO_RESIZE_UNDEFINED, so
> > AUTO_RESIZE_NEVER really is the default.
> > 
> > Fixes: 981b83ad1 ("multipathd: Add auto_resize config option")
> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> 
> The analysis is correct, but the fix differs from what we did for other
> options. We won't be able to cleanly change the default later on with
> this patch applied.
> 
> I would prefer to keep AUTO_RESIZE_UNDEF and DEFAULT_AUTO_RESIZE and
> just change the condition in uev_update_path() from "auto_resize !=
> AUTO_RESIZE_NEVER" to "(auto_resize == AUTO_RESIZE_GROW_ONLY ||
> auto_resize == AUTO_RESIZE_GROW_SHRINK)", unless you have strong
> reasons not to do so.

I would argue that by removing AUTO_RESIZE_UNDEF, I am making this
option more like our existing ones. Are there any options that we have
an UNDEF value for, where we don't need it to mean that we should use a
default which is defined someplace else? We either need UNDEF to show
that an option was not included in a devices or multipaths section, or
to mean that we should use a default system value defined outside of
multipath, like for dev_loss_tmo or max_fds. I don't see what it gets us
for an option like this where mulitpath must always have some defined
default behavior. There are many options that don't have and UNDEF
value. All the yes/no values that only exist in the defaults section, as
well as defaults section only options with multiple values like
force_reload and queue_without_daemon.

I'm fine with explicitly defining a DEFAULT value and setting it in
__init_config(). There are options where 0 is not UNDEF where don't do
this, like force_reload and queue_without_daemon (as well as most of the
ones where we only have yes/no values), and there are options where we
do explicitly set the option to 0, like reassign_maps and force_sync.
Manually setting the value to 0 is unnecessary, but I agree that it's
clearer to define it explicitly.

How does that sound (removing AUTO_RESIZE_UNDEF, but keeping and
actually setting the default)?

-Ben

> 
> Regards,
> Martin

Re: [PATCH 2/2] multipathd: fix auto-resize configuration

[Martin Wilck]

On Tue, 2024-01-16 at 14:03 -0500, Benjamin Marzinski wrote:
> 
> How does that sound (removing AUTO_RESIZE_UNDEF, but keeping and
> actually setting the default)?

That's fine.

Thanks,
Martin