netdev - Re: configfs: Create config

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZRccv2H3wK6PL5Rb@google.com>
Date: Fri, 29 Sep 2023 11:51:43 -0700
From: Joel Becker <jlbec@...lplan.org>
To: Breno Leitao <leitao@...ian.org>
Cc: hch@....de, netdev@...r.kernel.org
Subject: Re: configfs: Create config_item from netconsole

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@...lplan.org