build warning question - gcc 4.4.5

build warning question - gcc 4.4.5

[Nicholas Mc Guire]

HI !

 during Linux kernel build I got the following build warning

kernel/trace/trace.c: In function 'tracing_mark_write':
kernel/trace/trace.c:3818: warning: 'page2' may be used uninitialized in this function

 after looking at this function I was not able to figure out how
 this uninitialized usage could happen - all access to page2
 is "protected" by an "if (nr_pages == 2)" so no uninitialized
 access should be possible.

 is there some way to make gcc happy here ?

 full ref and code as well as gcc specs below.

thx!
hofrat

linux-stable v3.4.75 kernel/trace/trace.c

static ssize_t
tracing_mark_write(struct file *filp, const char __user *ubuf,
					size_t cnt, loff_t *fpos)
{
	unsigned long addr = (unsigned long)ubuf;
	struct ring_buffer_event *event;
	struct ring_buffer *buffer;
	struct print_entry *entry;
	unsigned long irq_flags;
	struct page *pages[2];
	int nr_pages = 1;
	ssize_t written;
	void *page1;
	void *page2;
	int offset;
	int size;
	int len;
	int ret;

	if (tracing_disabled)
		return -EINVAL;

	if (cnt > TRACE_BUF_SIZE)
		cnt = TRACE_BUF_SIZE;

	/*
	 * Userspace is injecting traces into the kernel trace buffer.
	 * We want to be as non intrusive as possible.
	 * To do so, we do not want to allocate any special buffers
	 * or take any locks, but instead write the userspace data
	 * straight into the ring buffer.
	 *
	 * First we need to pin the userspace buffer into memory,
	 * which, most likely it is, because it just referenced it.
	 * But there's no guarantee that it is. By using get_user_pages_fast()
	 * and kmap_atomic/kunmap_atomic() we can get access to the
	 * pages directly. We then write the data directly into the
	 * ring buffer.
	 */
	BUILD_BUG_ON(TRACE_BUF_SIZE >= PAGE_SIZE);

	/* check if we cross pages */
	if ((addr & PAGE_MASK) != ((addr + cnt) & PAGE_MASK))
		nr_pages = 2;

	offset = addr & (PAGE_SIZE - 1);
	addr &= PAGE_MASK;

	ret = get_user_pages_fast(addr, nr_pages, 0, pages);
	if (ret < nr_pages) {
		while (--ret >= 0)
			put_page(pages[ret]);
		written = -EFAULT;
		goto out;
	}

	page1 = kmap_atomic(pages[0]);
	if (nr_pages == 2)
		page2 = kmap_atomic(pages[1]);

	local_save_flags(irq_flags);
	size = sizeof(*entry) + cnt + 2; /* possible \n added */
	buffer = global_trace.buffer;
	event = trace_buffer_lock_reserve(buffer, TRACE_PRINT, size,
					  irq_flags, preempt_count());
	if (!event) {
		/* Ring buffer disabled, return as if not open for write */
		written = -EBADF;
		goto out_unlock;
	}

	entry = ring_buffer_event_data(event);
	entry->ip = _THIS_IP_;

	if (nr_pages == 2) {
		len = PAGE_SIZE - offset;
		memcpy(&entry->buf, page1 + offset, len);
		memcpy(&entry->buf[len], page2, cnt - len);
	} else
		memcpy(&entry->buf, page1 + offset, cnt);

	if (entry->buf[cnt - 1] != '\n') {
		entry->buf[cnt] = '\n';
		entry->buf[cnt + 1] = '\0';
	} else
		entry->buf[cnt] = '\0';

	ring_buffer_unlock_commit(buffer, event);

	written = cnt;

	*fpos += written;

 out_unlock:
	if (nr_pages == 2)
		kunmap_atomic(page2);
	kunmap_atomic(page1);
	while (nr_pages > 0)
		put_page(pages[--nr_pages]);
 out:
	return written;
}

Arch: x86_64
OS: Debian 6.0.6
GCC: default debian 6.0.6 gcc
Using built-in specs.
Target: x86_64-linux-gnu
Configured with: ../src/configure -v --with-pkgversion='Debian 4.4.5-8' --with-bugurl=file:///usr/share/doc/gcc-4.4/README.Bugs --enable-languages=c,c++,fortran,objc,obj-c++ --prefix=/usr --program-suffix=-4.4 --enable-shared --enable-multiarch --enable-linker-build-id --with-system-zlib --libexecdir=/usr/lib --without-included-gettext --enable-threads=posix --with-gxx-include-dir=/usr/include/c++/4.4 --libdir=/usr/lib --enable-nls --enable-clocale=gnu --enable-libstdcxx-debug --enable-objc-gc --with-arch-32=i586 --with-tune=generic --enable-checking=release --build=x86_64-linux-gnu --host=x86_64-linux-gnu --target=x86_64-linux-gnu
Thread model: posix
gcc version 4.4.5 (Debian 4.4.5-8) 

Re: build warning question - gcc 4.4.5

[Marc Glisse]

On Mon, 6 Jan 2014, Nicholas Mc Guire wrote:

> during Linux kernel build I got the following build warning
>
> kernel/trace/trace.c: In function 'tracing_mark_write':
> kernel/trace/trace.c:3818: warning: 'page2' may be used uninitialized in this function
>
> after looking at this function I was not able to figure out how
> this uninitialized usage could happen - all access to page2
> is "protected" by an "if (nr_pages == 2)" so no uninitialized
> access should be possible.

Note the word "may": gcc isn't able (at least at this optimization level) 
to prove that page2 is always initialized before use, so it warns.

> is there some way to make gcc happy here ?

Initialize page2 to 0 where you declare it?

-- 
Marc Glisse

Re: build warning question - gcc 4.4.5

[Andrew Haley]

On 01/06/2014 12:43 PM, Marc Glisse wrote:
> On Mon, 6 Jan 2014, Nicholas Mc Guire wrote:
> 
>> during Linux kernel build I got the following build warning
>>
>> kernel/trace/trace.c: In function 'tracing_mark_write':
>> kernel/trace/trace.c:3818: warning: 'page2' may be used uninitialized in this function
>>
>> after looking at this function I was not able to figure out how
>> this uninitialized usage could happen - all access to page2
>> is "protected" by an "if (nr_pages == 2)" so no uninitialized
>> access should be possible.
> 
> Note the word "may": gcc isn't able (at least at this optimization level) 
> to prove that page2 is always initialized before use, so it warns.
> 
>> is there some way to make gcc happy here ?
> 
> Initialize page2 to 0 where you declare it?

<aside>

I'm very nervous about advice like this in general: see
https://www.schneier.com/blog/archives/2008/05/random_number_b.html
http://anonscm.debian.org/viewvc/pkg-openssl/openssl/trunk/rand/md_rand.c?p2=%2Fopenssl%2Ftrunk%2Frand%2Fmd_rand.c&p1=openssl%2Ftrunk%2Frand%2Fmd_rand.c&r1=141&r2=140&view=diff&pathrev=141

This happened because the maintainer saw a warning about uninitialized
data and "fixed" the code.

But, in this specific case, I have no such worries.

</aside>

Andrew.