configfs: Create config_item from netconsole

configfs: Create config_item from netconsole

[Breno Leitao]

Right now there is a limitation in netconsole, where it is impossible to
disable or modify the target created from the command line parameter.
(netconsole=...).

"netconsole" cmdline parameter sets the remote IP, and if the remote IP
changes, the machine needs to be rebooted (with the new remote IP set in
the command line parameter).

I am planning to reuse the dynamic netconsole mechanism for
the 'command line' target, i.e., create a `cmdline` configfs_item for
the "command line" target, so, the user can modify the "command line"
target in configfs in runtime. Something as :

	echo 0 > /sys/kernel/config/netconsole/cmdline/enabled
	echo <new-IP> > /sys/kernel/config/netconsole/cmdline/remote_ip
	echo 1 > /sys/kernel/config/netconsole/cmdline/enabled

I didn't find a configfs API to register a configfs_item into a
config_group. Basically the make_item() callbacks are called once the
inode is created by the user at mkdir(2) time, but now I need to create
it at the driver initialization.

Should I create a configfs_register_item() to solve this problem?

Thanks!

Re: configfs: Create config_item from netconsole

[Joel Becker]

On Thu, Sep 28, 2023 at 07:44:58AM -0700, Breno Leitao wrote:
> Right now there is a limitation in netconsole, where it is impossible to
> disable or modify the target created from the command line parameter.
> (netconsole=...).
> 
> "netconsole" cmdline parameter sets the remote IP, and if the remote IP
> changes, the machine needs to be rebooted (with the new remote IP set in
> the command line parameter).
> 
> I am planning to reuse the dynamic netconsole mechanism for
> the 'command line' target, i.e., create a `cmdline` configfs_item for
> the "command line" target, so, the user can modify the "command line"
> target in configfs in runtime. Something as :
> 
> 	echo 0 > /sys/kernel/config/netconsole/cmdline/enabled
> 	echo <new-IP> > /sys/kernel/config/netconsole/cmdline/remote_ip
> 	echo 1 > /sys/kernel/config/netconsole/cmdline/enabled

Note that the `netconsole=` command line parameter can specify more than
one network console, split by semicolons.  Anything you do here has to be
responsive to this.  So you can't create a single `cmdline` entry.

> I didn't find a configfs API to register a configfs_item into a
> config_group. Basically the make_item() callbacks are called once the
> inode is created by the user at mkdir(2) time, but now I need to create
> it at the driver initialization.
>
> Should I create a configfs_register_item() to solve this problem?

It's an express philosophy of configfs that all lifetimes are controlled
by userspace, which is why we don't have such a facility.  If hch wants
to change this, I defer to his judgement.  But I don't think it is
necessary.

What I would do instead is check whether a mkdir(2) call is
trying to reference a command line entry.  If so, attach it to the
existing entry rather than creating a new one.

Currently, `alloc_param_target()` initializes `netconsole_target.item`
to zeros.  The item is never used by parameter-created targets.  Step
one would be to give it a name.  So in `init_netconsole()`, right after
`alloc_param_target()`, initialize `nt->item` just like we do in
`make_netconsole_target()`.  So something like:

```
+ #ifdef CONFIG_NETCONOLE_DYNAMIC
+       char target_name[16];
+       int target_count = 0;
+ #endif
	while ((target_config = strsep(&input, ";"))) {
			nt = alloc_param_target(target_config);
			if (IS_ERR(nt)) {
				err = PTR_ERR(nt);
				goto fail;
			}
+ #ifdef CONFIG_NETCONSOLE_DYNAMIC
+                       snprintf(target_name, 16, "cmdline", target_count);
+                       config_item_init_type_name(&nt->item, target_name,
+                                                  &netconsole_target_type);
+                       target_count++;
+ #endif
+
			/* Dump existing printks when we register */
			if (nt->extended) {
				extended = true;
```

Then, later in `make_netconsole_target()`, rather than blindly inserting
the new `netconsole_target` in the list, you can check if the name is
already present and use that.  Here's some ugly pseudocode:

```
	spin_lock_irqsave(&target_list_lock, flags);
	list_for_each_entry(tmp, &target_list, list) {
		if (!strcmp(tmp->item.name, nt->item.name)) {
			existing = tmp;
			break;
		}
	}
	if (existing) {
		to_free = nt;
		nt = existing;
	} else {
		list_add(&nt->list, &target_list);
	}
	spin_unlock_irqrestore(&target_list_lock, flags);

	if (to_free)
		kfree(to_free);

	return &nt->item;
}
```

In this fashion, each console created on the command line will get a
name of `cmdline0`, `cmdline1`, etc.  They will not be part of the
configfs tree.  If the user comes along later and says `mkdir
/sys/kernel/config/netconsole/cmdline1`, the existing `cmdline1` console
will be attached to the configfs tree.  The user is then free to disable
and reconfigure the device.

Note that this behavior cannot be triggered for netconsoles already
created by configfs.  If I try to `mkdir
/sys/kernel/config/netconsole/mydev` twice, the second command will get
`-EEXISTS` in filesystem code long before it reaches the netconsole
code.  Only when someone makes a config item that matches the console
names can we traverse this code.  Thus, matching the name is safe.

There would, of course, be some other corner cases to handle.  Do we
allow dynamic names that look like command-line names if no command-line
parameter exists?  Does `rmdir /sys/kernel/config/netconsole/cmdline0`
actually delete the command-line console entry, or does it return
-EBUSY?  And so on.

Thanks,
Joel

-- 

"When ideas fail, words come in very handy." 
         - Goethe

			http://www.jlbec.org/
			jlbec@evilplan.org

Re: configfs: Create config_item from netconsole

[Breno Leitao]

Hello Joel,

On Fri, Sep 29, 2023 at 11:51:43AM -0700, Joel Becker wrote:
> On Thu, Sep 28, 2023 at 07:44:58AM -0700, Breno Leitao wrote:

> > Should I create a configfs_register_item() to solve this problem?
> 
> It's an express philosophy of configfs that all lifetimes are controlled
> by userspace, which is why we don't have such a facility.  If hch wants
> to change this, I defer to his judgement.  But I don't think it is
> necessary.

I am happy the suggestion you gave, so, no need to change this
philosophy.

> In this fashion, each console created on the command line will get a
> name of `cmdline0`, `cmdline1`, etc.  They will not be part of the
> configfs tree.  If the user comes along later and says `mkdir
> /sys/kernel/config/netconsole/cmdline1`, the existing `cmdline1` console
> will be attached to the configfs tree.  The user is then free to disable
> and reconfigure the device.

I am happy with this approach.

> There would, of course, be some other corner cases to handle.  Do we
> allow dynamic names that look like command-line names if no command-line
> parameter exists? 

I'd say so, I think `cmdline` targets shouldn't be special if there are
not netconsole targets defined at boot time.


> Does `rmdir /sys/kernel/config/netconsole/cmdline0`
> actually delete the command-line console entry, or does it return
> -EBUSY?  And so on.

I've looked at the code and tested, it `cmdline0`could be removed as any
other target entry. On top of that, the user can create a new `cmdline0`
entry, and, it will be set with the default values for the attributes.

I am planning to send a patch based on what you suggested, and we can
continue the discuss there.

Thanks for the _very_ detailed feedback!
Breno