[Cluster-devel] [PATCH dlm-tool] dlm_controld: better uevent filtering

[Cluster-devel] [PATCH dlm-tool] dlm_controld: better uevent filtering

[Alexander Aring]

When I did test with dlm_locktorture module I got several log messages
about:

uevent message has 3 args: add@/module/dlm_locktorture
uevent message has 3 args: remove@/module/dlm_locktorture

which are not expected and not able to parse by dlm_controld
process_uevent() function, because mismatch of argument counts.
Debugging it more, I figured out that those uevents are for
loading/unloading the dlm_locktorture module and there are uevents for
loading and unloading modules which have nothing todo with dlm lockspace
uevent handling.

The current filter works as:

if (!strstr(buf, "dlm"))

for matching the dlm joining/leaving uevent string which looks like:

offline@/kernel/dlm/locktorture

to avoid matching with other uevent which has somehow the string "dlm"
in it, we switch to the match "/dlm/" which should match only to dlm
uevent system events. Uevent uses itself '/' as a separator in the hope
that uevents cannot put a '/' as application data for an event.
---
 dlm_controld/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/dlm_controld/main.c b/dlm_controld/main.c
index 7cf6348e..40689c5c 100644
--- a/dlm_controld/main.c
+++ b/dlm_controld/main.c
@@ -704,7 +704,7 @@ static void process_uevent(int ci)
 		return;
 	}
 
-	if (!strstr(buf, "dlm"))
+	if (!strstr(buf, "/dlm/"))
 		return;
 
 	log_debug("uevent: %s", buf);
-- 
2.31.1

[Cluster-devel] [PATCH dlm-tool] dlm_controld: better uevent filtering

[Steven Whitehouse]

Hi,

On Fri, 2023-01-13 at 17:43 -0500, Alexander Aring wrote:
> When I did test with dlm_locktorture module I got several log
> messages
> about:
> 
> uevent message has 3 args: add@/module/dlm_locktorture
> uevent message has 3 args: remove@/module/dlm_locktorture
> 
> which are not expected and not able to parse by dlm_controld
> process_uevent() function, because mismatch of argument counts.
> Debugging it more, I figured out that those uevents are for
> loading/unloading the dlm_locktorture module and there are uevents
> for
> loading and unloading modules which have nothing todo with dlm
> lockspace
> uevent handling.
> 
> The current filter works as:
> 
> if (!strstr(buf, "dlm"))
> 
I think that is the problem. If you want to filter out all events
except those sent by the DLM module, then you need to look at the
variables sent along with the request. Otherwise whatever string you
look for here might appear in some other random request from a
different subsystem. The event variables are much easier to parse than
the actual event string...

See a similar example in decode_uevent(), etc here:

https://github.com/andyprice/gfs2-utils/blob/91c3e9a69ed70d3d522f5b47015da5e5868722ec/group/gfs_controld/main.c

There are probably nicer ways of doing that, than what I did there, but
it makes it is easier to get at the variables that are passed with the
notification, than to try and parse the first line of the response,

Steve.


> for matching the dlm joining/leaving uevent string which looks like:
> 
> offline@/kernel/dlm/locktorture
> 
> to avoid matching with other uevent which has somehow the string
> "dlm"
> in it, we switch to the match "/dlm/" which should match only to dlm
> uevent system events. Uevent uses itself '/' as a separator in the
> hope
> that uevents cannot put a '/' as application data for an event.
> ---
> ?dlm_controld/main.c | 2 +-
> ?1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/dlm_controld/main.c b/dlm_controld/main.c
> index 7cf6348e..40689c5c 100644
> --- a/dlm_controld/main.c
> +++ b/dlm_controld/main.c
> @@ -704,7 +704,7 @@ static void process_uevent(int ci)
> ????????????????return;
> ????????}
> ?
> -???????if (!strstr(buf, "dlm"))
> +???????if (!strstr(buf, "/dlm/"))
> ????????????????return;
> ?
> ????????log_debug("uevent: %s", buf);

[Cluster-devel] [PATCH dlm-tool] dlm_controld: better uevent filtering

[Alexander Aring]

Hi,

On Mon, Jan 16, 2023 at 5:36 AM Steven Whitehouse <swhiteho@redhat.com> wrote:
>
> Hi,
>
> On Fri, 2023-01-13 at 17:43 -0500, Alexander Aring wrote:
> > When I did test with dlm_locktorture module I got several log
> > messages
> > about:
> >
> > uevent message has 3 args: add@/module/dlm_locktorture
> > uevent message has 3 args: remove@/module/dlm_locktorture
> >
> > which are not expected and not able to parse by dlm_controld
> > process_uevent() function, because mismatch of argument counts.
> > Debugging it more, I figured out that those uevents are for
> > loading/unloading the dlm_locktorture module and there are uevents
> > for
> > loading and unloading modules which have nothing todo with dlm
> > lockspace
> > uevent handling.
> >
> > The current filter works as:
> >
> > if (!strstr(buf, "dlm"))
> >
> I think that is the problem. If you want to filter out all events
> except those sent by the DLM module, then you need to look at the
> variables sent along with the request. Otherwise whatever string you
> look for here might appear in some other random request from a
> different subsystem. The event variables are much easier to parse than
> the actual event string...
>
> See a similar example in decode_uevent(), etc here:
>
> https://github.com/andyprice/gfs2-utils/blob/91c3e9a69ed70d3d522f5b47015da5e5868722ec/group/gfs_controld/main.c
>
> There are probably nicer ways of doing that, than what I did there, but
> it makes it is easier to get at the variables that are passed with the
> notification, than to try and parse the first line of the response,

This requires us to switch to the kernel uevent trigger function call
from kobject_uevent() [0] to kobject_uevent_env() [1], because we
don't have those envs like SUBSYSTEM being sent right now.
I was curious because I was sure that I printed out the "raw string"
from udev netlink socket and didn't see those envs, otherwise I
probably would use it if I saw those. Now I figured out we need to
have a UAPI change for that.

Unfortunately, for me it looks like older dlm_controld versions cannot
handle such change.

- Alex

[0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/dlm/lockspace.c?h=v6.2-rc4#n200
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/lib/kobject_uevent.c?h=v6.2-rc4#n447

[Cluster-devel] [PATCH dlm-tool] dlm_controld: better uevent filtering

[Alexander Aring]

Hi,

On Mon, Jan 16, 2023 at 9:26 AM Alexander Aring <aahringo@redhat.com> wrote:
>
> Hi,
>
> On Mon, Jan 16, 2023 at 5:36 AM Steven Whitehouse <swhiteho@redhat.com> wrote:
> >
> > Hi,
> >
> > On Fri, 2023-01-13 at 17:43 -0500, Alexander Aring wrote:
> > > When I did test with dlm_locktorture module I got several log
> > > messages
> > > about:
> > >
> > > uevent message has 3 args: add@/module/dlm_locktorture
> > > uevent message has 3 args: remove@/module/dlm_locktorture
> > >
> > > which are not expected and not able to parse by dlm_controld
> > > process_uevent() function, because mismatch of argument counts.
> > > Debugging it more, I figured out that those uevents are for
> > > loading/unloading the dlm_locktorture module and there are uevents
> > > for
> > > loading and unloading modules which have nothing todo with dlm
> > > lockspace
> > > uevent handling.
> > >
> > > The current filter works as:
> > >
> > > if (!strstr(buf, "dlm"))
> > >
> > I think that is the problem. If you want to filter out all events
> > except those sent by the DLM module, then you need to look at the
> > variables sent along with the request. Otherwise whatever string you
> > look for here might appear in some other random request from a
> > different subsystem. The event variables are much easier to parse than
> > the actual event string...
> >
> > See a similar example in decode_uevent(), etc here:
> >
> > https://github.com/andyprice/gfs2-utils/blob/91c3e9a69ed70d3d522f5b47015da5e5868722ec/group/gfs_controld/main.c
> >
> > There are probably nicer ways of doing that, than what I did there, but
> > it makes it is easier to get at the variables that are passed with the
> > notification, than to try and parse the first line of the response,
>
> This requires us to switch to the kernel uevent trigger function call
> from kobject_uevent() [0] to kobject_uevent_env() [1], because we
> don't have those envs like SUBSYSTEM being sent right now.
> I was curious because I was sure that I printed out the "raw string"
> from udev netlink socket and didn't see those envs, otherwise I
> probably would use it if I saw those. Now I figured out we need to
> have a UAPI change for that.
>
> Unfortunately, for me it looks like older dlm_controld versions cannot
> handle such change.

after Steven mentioned to me that kobject_uevent() calls
kobject_uevent_env(), I saw that there is the SUBSYSTEM environment,
etc. in there after the first nul terminated string.
I will send a v2 based on the gfs2_controld parser to not parse the
first "offline@/kernel/dlm/locktorture" string anymore and get each
field by its env which works as a key-value pair.

- Alex