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
| ||
|
Date: Fri, 2 Sep 2016 09:34:23 +0800 From: Xishi Qiu <qiuxishi@...wei.com> To: Andrew Morton <akpm@...ux-foundation.org> CC: Reza Arbab <arbab@...ux.vnet.ibm.com>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Vlastimil Babka <vbabka@...e.cz>, "Vitaly Kuznetsov" <vkuznets@...hat.com>, David Rientjes <rientjes@...gle.com>, "Yaowei Bai" <baiyaowei@...s.chinamobile.com>, Joonsoo Kim <iamjoonsoo.kim@....com>, Dan Williams <dan.j.williams@...el.com>, David Vrabel <david.vrabel@...rix.com>, Chen Yucong <slaoub@...il.com>, Andrew Banman <abanman@....com>, Seth Jennings <sjenning@...hat.com>, <linux-mm@...ck.org>, <linux-kernel@...r.kernel.org> Subject: Re: [PATCH v3] memory-hotplug: fix store_mem_state() return value On 2016/9/2 4:37, Andrew Morton wrote: > On Thu, 1 Sep 2016 10:29:37 -0500 Reza Arbab <arbab@...ux.vnet.ibm.com> wrote: > >> If store_mem_state() is called to online memory which is already online, >> it will return 1, the value it got from device_online(). >> >> This is wrong because store_mem_state() is a device_attribute .store >> function. Thus a non-negative return value represents input bytes read. >> >> Set the return value to -EINVAL in this case. >> > > I actually made the mistake of reading this code. > > What the heck are the return value semantics of bus_type.online? > Sometimes 0, sometimes 1 and apparently sometimes -Efoo values. What > are these things trying to tell the caller and why is "1" ever useful > and why doesn't anyone document anything. grr. > > And now I don't understand this patch. Because: > > static int memory_subsys_online(struct device *dev) > { > struct memory_block *mem = to_memory_block(dev); > int ret; > > if (mem->state == MEM_ONLINE) > return 0; > I think we will not execute here, it will return from device_online(), because "if (dev->offline)" is false and return 1. But the two return vaules are different if we do online-to-online. memory_subsys_online() return 0, and device_online() return 1, this is a little confusion. When device_online() return 1, online_store() return 1 and store_mem_state() return -EINVAL even without this patch, as Reza described in v2. 1. store_mem_state() called with buf="online" 2. device_online() returns 1 because device is already online 3. store_mem_state() returns 1 4. calling code interprets this as 1-byte buffer read 5. store_mem_state() called again with buf="nline" 6. store_mem_state() returns -EINVAL Thanks, Xishi Qiu > Doesn't that "return 0" contradict the changelog? > > Also, is store_mem_state() the correct place to fix this? Instead, > should memory_block_change_state() detect an attempt to online > already-online memory and itself return -EINVAL, and permit that to be > propagated back? Well, that depends on the bus_type.online rules which > appear to be undocumented. What is the bus implementation supposed to > do when a request is made to online an already-online device? > > > > . >
Powered by blists - more mailing lists