netdev - Re: [PATCH -mm 9/9] netconsole: Support dynamic reconfiguration using configfs

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]

Date:	Wed, 4 Jul 2007 18:51:36 +0530 (IST)
From:	Satyam Sharma <ssatyam@....iitk.ac.in>
To:	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
cc:	Keiichi Kii <k-keiichi@...jp.nec.com>,
	Netdev <netdev@...r.kernel.org>,
	Joel Becker <joel.becker@...cle.com>,
	Matt Mackall <mpm@...enic.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	David Miller <davem@...emloft.net>
Subject: Re: [PATCH -mm 9/9] netconsole: Support dynamic reconfiguration
 using configfs

[ Some more thoughts I had on this, for those interested. ]

> +#ifdef	CONFIG_NETCONSOLE_DYNAMIC
> +
> +/*
> + * Targets that were created by parsing the boot/module option string
> + * do not exist in the configfs hierarchy and will never go away (and
> + * have zeroed-out config_item members). So make these a no-op for them.
> + */
> +static void netconsole_target_get(struct netconsole_target *nt)
> +{
> +	static struct config_item empty_item;	/* Zeroed-out config_item */
> +
> +	if (memcmp(&nt->item, &empty_item, sizeof(struct config_item)))
> +		config_item_get(&nt->item);
> +}
> +
> +static void netconsole_target_put(struct netconsole_target *nt)
> +{
> +	static struct config_item empty_item;	/* Zeroed-out config_item */
> +
> +	if (memcmp(&nt->item, &empty_item, sizeof(struct config_item)))
> +		config_item_put(&nt->item);
> +}
> +
> +#else	/* !CONFIG_NETCONSOLE_DYNAMIC */
> +
>  /*
> - * Allocate new target and setup netpoll for it.
> + * No danger of targets going away from under us when dynamic
> + * reconfigurability is off.
> + */
> +static void netconsole_target_get(struct netconsole_target *nt)
> +{
> +}
> +
> +static void netconsole_target_put(struct netconsole_target *nt)
> +{
> +}
> +
> +#endif	/* CONFIG_NETCONSOLE_DYNAMIC */

This whole stuff (and calls to netconsole_target_get and _put from code
below) is not necessary. All the access to the netconsole_target objects
(that are present in the target_list) happens inside
spin_lock_irqsave(target_list_lock), so there's no need to do a
config_item_get() on these as there is no race window in which userspace
can delete (and thus release) these items from under us. Note that
drop_netconsole_target() removes target-to-delete from target_list
(under spin_lock_irqsave) _before_ free'ing the objects, and that both
_get() and _put() are balanced *inside* a spin_lock_irqsave /
spin_unlock_irqrestore pair -- so all this business is quite pointless.

I've kept this stuff in for now, but would like to get rid of it, really.
It's an unnecessary #ifdef-#else-#endif kludge. Comments?

> +	if (nt->enabled) {
> +		printk(KERN_ERR "netconsole: target (id %d) is enabled, "
> +				"disable to update parameters\n", nt->id);
> +		return -EINVAL;
> +	}

[ Such code exists in all the store() operations for all attributes other
than "enabled" itself. ]

Anyway, the problem is that the error does not show up violently in the
shell. The write fails, of course, values are unchanged, and dmesg shows
the diagnostic outputted above -- but "echo" does not complain about
-EINVAL returning from write(2) for some reason, neither did -EACCES.
I guess "echo" needs to be fixed. BTW would -EACCES be more appropriate?

Satyam
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html