linux-kernel - Re: [PATCH v5 1/1] KVM: trigger uevents when creating or destroying a VM

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: <20170717182756.33ed1959@p-imbrenda.boeblingen.de.ibm.com>
Date:   Mon, 17 Jul 2017 18:27:56 +0200
From:   Claudio Imbrenda <imbrenda@...ux.vnet.ibm.com>
To:     David Hildenbrand <david@...hat.com>
Cc:     kvm@...r.kernel.org, borntraeger@...ibm.com, pbonzini@...hat.com,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 1/1] KVM: trigger uevents when creating or destroying
 a VM

On Mon, 17 Jul 2017 17:53:51 +0200
David Hildenbrand <david@...hat.com> wrote:

> > +	add_uevent_var(env, "CREATED=%llu", created);
> > +	add_uevent_var(env, "COUNT=%llu", active);  
> 
> I like that much better.
> 
> > +
> > +	if (type == KVM_EVENT_CREATE_VM)
> > +		add_uevent_var(env, "EVENT=create");
> > +	else if (type == KVM_EVENT_DESTROY_VM)
> > +		add_uevent_var(env, "EVENT=destroy");
> > +
> > +	if (kvm->debugfs_dentry) {
> > +		char p[ITOA_MAX_LEN];  
> 
> I'd move this also to the top of the function. Or move tmp and pathbuf
> in here, so you could also move them to this place.

yeah it would probably look cleaner, I'll fix it.

> > +
> > +		snprintf(p, sizeof(p), "%s",
> > kvm->debugfs_dentry->d_name.name);  
> 
> Probably just me, but I prefer ARRAY_SIZE() instead of sizeof() for
> any kind of array sizes.

I'll fix that too

> > +		tmp = strchrnul(p + 1, '-');> +
> > *tmp = '\0';
> > +		add_uevent_var(env, "PID=%s", p);
> > +		pathbuf = kmalloc(PATH_MAX, GFP_KERNEL);
> > +		if (pathbuf) {
> > +			/* sizeof counts the final '\0' */
> > +			int len = sizeof("STATS_PATH=") - 1;
> > +			const char *pvar = "STATS_PATH=";  
> 
> Can't you avoid that? See next paragraph.
> 
> 
> > +
> > +			tmp = dentry_path_raw(kvm->debugfs_dentry,
> > +					      pathbuf + len,
> > +					      PATH_MAX - len);
> > +			if (!IS_ERR(tmp)) {  
> 
> Why not
> 
> tmp = dentry_path_raw(kvm->debugfs_dentry, pathbuf, PATH_MAX);
> if (!IS_ERR(tmp)) {
> 	add_uevent_var(env, "STATS_PATH=s", tmp);
> }
> 
> and avoid len / pvar / memcpy? And keep internal env size checking
> consistent.

that would be true, but then we would copy the buffer with the path
twice, once for dentry_path_raw and once for add_uevent_var.

the env size is consistent, since that would count the bytes
effectively present in the buffer, and note that kobject_uevent_env only
wants a char**, no length, so there are no consistency issues. 

I agree that your solution looks better (and I had actually considered
implementing it like that initially), but it can potentially be slower.
I don't really have any strong preference.

Paolo: which solution do you like the most? 

[...]