Re: fs/binfmt_flat.c:828:9: error: void value not ignored as it ought to be

[Randy Dunlap]

On 08/19/2017 04:27 PM, kbuild test robot wrote:
> Hi Al,
> 
> FYI, the error/warning still remains.
> 
> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> head:   58d4e450a490d5f02183f6834c12550ba26d3b47
> commit: 468138d78510688fb5476f98d23f11ac6a63229a binfmt_flat: flat_{get,put}_addr_from_rp() should be able to fail
> date:   7 weeks ago
> config: m32r-mappi.nommu_defconfig (attached as .config)
> compiler: m32r-linux-gcc (GCC) 6.2.0
> reproduce:
>         wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         git checkout 468138d78510688fb5476f98d23f11ac6a63229a
>         # save the attached .config to linux build tree
>         make.cross ARCH=m32r 

Hi Al,

I sent a patch to m322 and microblaze a few weeks ago for this build error.
(July 30, 2017)  https://lkml.org/lkml/2017/7/30/179

Have you had a chance to look at it?

Thanks.

> All errors (new ones prefixed by >>):
> 
>>> fs/binfmt_flat.c:828:9: error: void value not ignored as it ought to be
>         ret = flat_put_addr_at_rp(rp, addr, relval);
>             ^
> 
> vim +828 fs/binfmt_flat.c
> 
>    506	
>    507		/*
>    508		 * Check initial limits. This avoids letting people circumvent
>    509		 * size limits imposed on them by creating programs with large
>    510		 * arrays in the data or bss.
>    511		 */
>    512		rlim = rlimit(RLIMIT_DATA);
>    513		if (rlim >= RLIM_INFINITY)
>    514			rlim = ~0;
>    515		if (data_len + bss_len > rlim) {
>    516			ret = -ENOMEM;
>    517			goto err;
>    518		}
>    519	
>    520		/* Flush all traces of the currently running executable */
>    521		if (id == 0) {
>    522			ret = flush_old_exec(bprm);
>    523			if (ret)
>    524				goto err;
>    525	
>    526			/* OK, This is the point of no return */
>    527			set_personality(PER_LINUX_32BIT);
>    528			setup_new_exec(bprm);
>    529		}
>    530	
>    531		/*
>    532		 * calculate the extra space we need to map in
>    533		 */
>    534		extra = max_t(unsigned long, bss_len + stack_len,
>    535				relocs * sizeof(unsigned long));
>    536	
>    537		/*
>    538		 * there are a couple of cases here,  the separate code/data
>    539		 * case,  and then the fully copied to RAM case which lumps
>    540		 * it all together.
>    541		 */
>    542		if (!IS_ENABLED(CONFIG_MMU) && !(flags & (FLAT_FLAG_RAM|FLAT_FLAG_GZIP))) {
>    543			/*
>    544			 * this should give us a ROM ptr,  but if it doesn't we don't
>    545			 * really care
>    546			 */
>    547			pr_debug("ROM mapping of file (we hope)\n");
>    548	
>    549			textpos = vm_mmap(bprm->file, 0, text_len, PROT_READ|PROT_EXEC,
>    550					  MAP_PRIVATE|MAP_EXECUTABLE, 0);
>    551			if (!textpos || IS_ERR_VALUE(textpos)) {
>    552				ret = textpos;
>    553				if (!textpos)
>    554					ret = -ENOMEM;
>    555				pr_err("Unable to mmap process text, errno %d\n", ret);
>    556				goto err;
>    557			}
>    558	
>    559			len = data_len + extra + MAX_SHARED_LIBS * sizeof(unsigned long);
>    560			len = PAGE_ALIGN(len);
>    561			realdatastart = vm_mmap(NULL, 0, len,
>    562				PROT_READ|PROT_WRITE|PROT_EXEC, MAP_PRIVATE, 0);
>    563	
>    564			if (realdatastart == 0 || IS_ERR_VALUE(realdatastart)) {
>    565				ret = realdatastart;
>    566				if (!realdatastart)
>    567					ret = -ENOMEM;
>    568				pr_err("Unable to allocate RAM for process data, "
>    569				       "errno %d\n", ret);
>    570				vm_munmap(textpos, text_len);
>    571				goto err;
>    572			}
>    573			datapos = ALIGN(realdatastart +
>    574					MAX_SHARED_LIBS * sizeof(unsigned long),
>    575					FLAT_DATA_ALIGN);
>    576	
>    577			pr_debug("Allocated data+bss+stack (%ld bytes): %lx\n",
>    578				 data_len + bss_len + stack_len, datapos);
>    579	
>    580			fpos = ntohl(hdr->data_start);
>    581	#ifdef CONFIG_BINFMT_ZFLAT
>    582			if (flags & FLAT_FLAG_GZDATA) {
>    583				result = decompress_exec(bprm, fpos, (char *)datapos,
>    584							 full_data, 0);
>    585			} else
>    586	#endif
>    587			{
>    588				result = read_code(bprm->file, datapos, fpos,
>    589						full_data);
>    590			}
>    591			if (IS_ERR_VALUE(result)) {
>    592				ret = result;
>    593				pr_err("Unable to read data+bss, errno %d\n", ret);
>    594				vm_munmap(textpos, text_len);
>    595				vm_munmap(realdatastart, len);
>    596				goto err;
>    597			}
>    598	
>    599			reloc = (u32 __user *)
>    600				(datapos + (ntohl(hdr->reloc_start) - text_len));
>    601			memp = realdatastart;
>    602			memp_size = len;
>    603		} else {
>    604	
>    605			len = text_len + data_len + extra + MAX_SHARED_LIBS * sizeof(u32);
>    606			len = PAGE_ALIGN(len);
>    607			textpos = vm_mmap(NULL, 0, len,
>    608				PROT_READ | PROT_EXEC | PROT_WRITE, MAP_PRIVATE, 0);
>    609	
>    610			if (!textpos || IS_ERR_VALUE(textpos)) {
>    611				ret = textpos;
>    612				if (!textpos)
>    613					ret = -ENOMEM;
>    614				pr_err("Unable to allocate RAM for process text/data, "
>    615				       "errno %d\n", ret);
>    616				goto err;
>    617			}
>    618	
>    619			realdatastart = textpos + ntohl(hdr->data_start);
>    620			datapos = ALIGN(realdatastart +
>    621					MAX_SHARED_LIBS * sizeof(u32),
>    622					FLAT_DATA_ALIGN);
>    623	
>    624			reloc = (u32 __user *)
>    625				(datapos + (ntohl(hdr->reloc_start) - text_len));
>    626			memp = textpos;
>    627			memp_size = len;
>    628	#ifdef CONFIG_BINFMT_ZFLAT
>    629			/*
>    630			 * load it all in and treat it like a RAM load from now on
>    631			 */
>    632			if (flags & FLAT_FLAG_GZIP) {
>    633	#ifndef CONFIG_MMU
>    634				result = decompress_exec(bprm, sizeof(struct flat_hdr),
>    635						 (((char *)textpos) + sizeof(struct flat_hdr)),
>    636						 (text_len + full_data
>    637							  - sizeof(struct flat_hdr)),
>    638						 0);
>    639				memmove((void *) datapos, (void *) realdatastart,
>    640						full_data);
>    641	#else
>    642				/*
>    643				 * This is used on MMU systems mainly for testing.
>    644				 * Let's use a kernel buffer to simplify things.
>    645				 */
>    646				long unz_text_len = text_len - sizeof(struct flat_hdr);
>    647				long unz_len = unz_text_len + full_data;
>    648				char *unz_data = vmalloc(unz_len);
>    649				if (!unz_data) {
>    650					result = -ENOMEM;
>    651				} else {
>    652					result = decompress_exec(bprm, sizeof(struct flat_hdr),
>    653								 unz_data, unz_len, 0);
>    654					if (result == 0 &&
>    655					    (copy_to_user((void __user *)textpos + sizeof(struct flat_hdr),
>    656							  unz_data, unz_text_len) ||
>    657					     copy_to_user((void __user *)datapos,
>    658							  unz_data + unz_text_len, full_data)))
>    659						result = -EFAULT;
>    660					vfree(unz_data);
>    661				}
>    662	#endif
>    663			} else if (flags & FLAT_FLAG_GZDATA) {
>    664				result = read_code(bprm->file, textpos, 0, text_len);
>    665				if (!IS_ERR_VALUE(result)) {
>    666	#ifndef CONFIG_MMU
>    667					result = decompress_exec(bprm, text_len, (char *) datapos,
>    668							 full_data, 0);
>    669	#else
>    670					char *unz_data = vmalloc(full_data);
>    671					if (!unz_data) {
>    672						result = -ENOMEM;
>    673					} else {
>    674						result = decompress_exec(bprm, text_len,
>    675							       unz_data, full_data, 0);
>    676						if (result == 0 &&
>    677						    copy_to_user((void __user *)datapos,
>    678								 unz_data, full_data))
>    679							result = -EFAULT;
>    680						vfree(unz_data);
>    681					}
>    682	#endif
>    683				}
>    684			} else
>    685	#endif /* CONFIG_BINFMT_ZFLAT */
>    686			{
>    687				result = read_code(bprm->file, textpos, 0, text_len);
>    688				if (!IS_ERR_VALUE(result))
>    689					result = read_code(bprm->file, datapos,
>    690							   ntohl(hdr->data_start),
>    691							   full_data);
>    692			}
>    693			if (IS_ERR_VALUE(result)) {
>    694				ret = result;
>    695				pr_err("Unable to read code+data+bss, errno %d\n", ret);
>    696				vm_munmap(textpos, text_len + data_len + extra +
>    697					MAX_SHARED_LIBS * sizeof(u32));
>    698				goto err;
>    699			}
>    700		}
>    701	
>    702		start_code = textpos + sizeof(struct flat_hdr);
>    703		end_code = textpos + text_len;
>    704		text_len -= sizeof(struct flat_hdr); /* the real code len */
>    705	
>    706		/* The main program needs a little extra setup in the task structure */
>    707		if (id == 0) {
>    708			current->mm->start_code = start_code;
>    709			current->mm->end_code = end_code;
>    710			current->mm->start_data = datapos;
>    711			current->mm->end_data = datapos + data_len;
>    712			/*
>    713			 * set up the brk stuff, uses any slack left in data/bss/stack
>    714			 * allocation.  We put the brk after the bss (between the bss
>    715			 * and stack) like other platforms.
>    716			 * Userspace code relies on the stack pointer starting out at
>    717			 * an address right at the end of a page.
>    718			 */
>    719			current->mm->start_brk = datapos + data_len + bss_len;
>    720			current->mm->brk = (current->mm->start_brk + 3) & ~3;
>    721	#ifndef CONFIG_MMU
>    722			current->mm->context.end_brk = memp + memp_size - stack_len;
>    723	#endif
>    724		}
>    725	
>    726		if (flags & FLAT_FLAG_KTRACE) {
>    727			pr_info("Mapping is %lx, Entry point is %x, data_start is %x\n",
>    728				textpos, 0x00ffffff&ntohl(hdr->entry), ntohl(hdr->data_start));
>    729			pr_info("%s %s: TEXT=%lx-%lx DATA=%lx-%lx BSS=%lx-%lx\n",
>    730				id ? "Lib" : "Load", bprm->filename,
>    731				start_code, end_code, datapos, datapos + data_len,
>    732				datapos + data_len, (datapos + data_len + bss_len + 3) & ~3);
>    733		}
>    734	
>    735		/* Store the current module values into the global library structure */
>    736		libinfo->lib_list[id].start_code = start_code;
>    737		libinfo->lib_list[id].start_data = datapos;
>    738		libinfo->lib_list[id].start_brk = datapos + data_len + bss_len;
>    739		libinfo->lib_list[id].text_len = text_len;
>    740		libinfo->lib_list[id].loaded = 1;
>    741		libinfo->lib_list[id].entry = (0x00ffffff & ntohl(hdr->entry)) + textpos;
>    742		libinfo->lib_list[id].build_date = ntohl(hdr->build_date);
>    743	
>    744		/*
>    745		 * We just load the allocations into some temporary memory to
>    746		 * help simplify all this mumbo jumbo
>    747		 *
>    748		 * We've got two different sections of relocation entries.
>    749		 * The first is the GOT which resides at the beginning of the data segment
>    750		 * and is terminated with a -1.  This one can be relocated in place.
>    751		 * The second is the extra relocation entries tacked after the image's
>    752		 * data segment. These require a little more processing as the entry is
>    753		 * really an offset into the image which contains an offset into the
>    754		 * image.
>    755		 */
>    756		if (flags & FLAT_FLAG_GOTPIC) {
>    757			for (rp = (u32 __user *)datapos; ; rp++) {
>    758				u32 addr, rp_val;
>    759				if (get_user(rp_val, rp))
>    760					return -EFAULT;
>    761				if (rp_val == 0xffffffff)
>    762					break;
>    763				if (rp_val) {
>    764					addr = calc_reloc(rp_val, libinfo, id, 0);
>    765					if (addr == RELOC_FAILED) {
>    766						ret = -ENOEXEC;
>    767						goto err;
>    768					}
>    769					if (put_user(addr, rp))
>    770						return -EFAULT;
>    771				}
>    772			}
>    773		}
>    774	
>    775		/*
>    776		 * Now run through the relocation entries.
>    777		 * We've got to be careful here as C++ produces relocatable zero
>    778		 * entries in the constructor and destructor tables which are then
>    779		 * tested for being not zero (which will always occur unless we're
>    780		 * based from address zero).  This causes an endless loop as __start
>    781		 * is at zero.  The solution used is to not relocate zero addresses.
>    782		 * This has the negative side effect of not allowing a global data
>    783		 * reference to be statically initialised to _stext (I've moved
>    784		 * __start to address 4 so that is okay).
>    785		 */
>    786		if (rev > OLD_FLAT_VERSION) {
>    787			u32 __maybe_unused persistent = 0;
>    788			for (i = 0; i < relocs; i++) {
>    789				u32 addr, relval;
>    790	
>    791				/*
>    792				 * Get the address of the pointer to be
>    793				 * relocated (of course, the address has to be
>    794				 * relocated first).
>    795				 */
>    796				if (get_user(relval, reloc + i))
>    797					return -EFAULT;
>    798				relval = ntohl(relval);
>    799				if (flat_set_persistent(relval, &persistent))
>    800					continue;
>    801				addr = flat_get_relocate_addr(relval);
>    802				rp = (u32 __user *)calc_reloc(addr, libinfo, id, 1);
>    803				if (rp == (u32 __user *)RELOC_FAILED) {
>    804					ret = -ENOEXEC;
>    805					goto err;
>    806				}
>    807	
>    808				/* Get the pointer's value.  */
>    809				ret = flat_get_addr_from_rp(rp, relval, flags,
>    810								&addr, &persistent);
>    811				if (unlikely(ret))
>    812					goto err;
>    813	
>    814				if (addr != 0) {
>    815					/*
>    816					 * Do the relocation.  PIC relocs in the data section are
>    817					 * already in target order
>    818					 */
>    819					if ((flags & FLAT_FLAG_GOTPIC) == 0)
>    820						addr = ntohl(addr);
>    821					addr = calc_reloc(addr, libinfo, id, 0);
>    822					if (addr == RELOC_FAILED) {
>    823						ret = -ENOEXEC;
>    824						goto err;
>    825					}
>    826	
>    827					/* Write back the relocated pointer.  */
>  > 828					ret = flat_put_addr_at_rp(rp, addr, relval);
>    829					if (unlikely(ret))
>    830						goto err;
>    831				}
>    832			}


-- 
~Randy