Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1311523 > unrolled thread

Re: [PATCH v3] kernel: add kcov code coverage

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3] kernel: add kcov code coverage "Kirill A. Shutemov" <kirill@shutemov.name> - 2016-01-18 13:50 +0100
    Re: [PATCH v3] kernel: add kcov code coverage Dmitry Vyukov <dvyukov@google.com> - 2016-01-18 20:30 +0100
      Re: [PATCH v3] kernel: add kcov code coverage "Kirill A. Shutemov" <kirill@shutemov.name> - 2016-01-18 23:00 +0100
        Re: [PATCH v3] kernel: add kcov code coverage Dmitry Vyukov <dvyukov@google.com> - 2016-01-19 13:50 +0100

#1311523 — Re: [PATCH v3] kernel: add kcov code coverage

On Thu, Jan 14, 2016 at 03:22:21PM +0100, Dmitry Vyukov wrote:
> +/*
> + * Entry point from instrumented code.
> + * This is called once per basic-block/edge.
> + */
> +void __sanitizer_cov_trace_pc(void)
> +{
> +	struct task_struct *t;
> +	enum kcov_mode mode;
> +
> +	t = current;
> +	/*
> +	 * We are interested in code coverage as a function of a syscall inputs,
> +	 * so we ignore code executed in interrupts.
> +	 */
> +	if (!t || in_interrupt())
> +		return;
> +	mode = READ_ONCE(t->kcov_mode);
> +	if (mode == kcov_mode_trace) {
> +		u32 *area;
> +		u32 pos;
> +
> +		/*
> +		 * There is some code that runs in interrupts but for which
> +		 * in_interrupt() returns false (e.g. preempt_schedule_irq()).
> +		 * READ_ONCE()/barrier() effectively provides load-acquire wrt
> +		 * interrupts, there are paired barrier()/WRITE_ONCE() in
> +		 * kcov_ioctl_locked().
> +		 */
> +		barrier();
> +		area = t->kcov_area;
> +		/* The first u32 is number of subsequent PCs. */
> +		pos = READ_ONCE(area[0]) + 1;
> +		if (likely(pos < t->kcov_size)) {
> +			area[pos] = (u32)_RET_IP_;

If area[0] is UINT32_MAX the condition will be true and you'll make
area[0] temporary set to (u32)_RET_IP_. That's may be fun. :)

> +			WRITE_ONCE(area[0], pos);
> +		}
> +	}
> +}
> +EXPORT_SYMBOL(__sanitizer_cov_trace_pc);

...

> +static int kcov_vm_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
> +{
> +	struct kcov *kcov;
> +	unsigned long off;
> +	struct page *page;
> +
> +	/* Map the preallocated kcov->area. */
> +	kcov = vma->vm_file->private_data;
> +	off = vmf->pgoff << PAGE_SHIFT;
> +	if (off >= kcov->size * sizeof(u32))
> +		return VM_FAULT_SIGSEGV;

SIGBUS.

> +
> +	page = vmalloc_to_page(kcov->area + off);
> +	get_page(page);
> +	vmf->page = page;
> +	return 0;
> +}

BTW, since all pages pre-allocated, you don't really need fault handler --
just setup all pages table enties during mmap. This would shorten warm up
period for userspace. See vm_insert_page().

> +
> +static void kcov_unmap(struct vm_area_struct *vma)
> +{
> +	kcov_put(vma->vm_file->private_data);
> +}
> +
> +static void kcov_map_copied(struct vm_area_struct *vma)
> +{
> +	struct kcov *kcov;
> +
> +	kcov = vma->vm_file->private_data;
> +	kcov_get(kcov);
> +}
> +
> +static const struct vm_operations_struct kcov_vm_ops = {
> +	.fault = kcov_vm_fault,
> +	.close = kcov_unmap,
> +	/* Called on fork()/clone() when the mapping is copied. */
> +	.open  = kcov_map_copied,
> +};
> +
> +static int kcov_mmap(struct file *filep, struct vm_area_struct *vma)
> +{
> +	int res = 0;
> +	void *area;
> +	struct kcov *kcov = vma->vm_file->private_data;
> +
> +	area = vmalloc_user(vma->vm_end - vma->vm_start);
> +	if (!area)
> +		return -ENOMEM;
> +
> +	spin_lock(&kcov->lock);
> +	if (kcov->mode == 0 || vma->vm_pgoff != 0 ||
> +	    vma->vm_end - vma->vm_start != kcov->size * sizeof(u32)) {

You need VM_DONTEXPAND to keep the invariant. Not sure about VM_DONTDUMP.

> +		res = -EINVAL;
> +		goto exit;
> +	}
> +	if (!kcov->area) {
> +		kcov->area = area;
> +		area = NULL;
> +	}
> +	/*
> +	 * The file drops a reference on close, but the file
> +	 * descriptor can be closed with the mmaping still alive so we keep
> +	 * a reference for those.  This is put in kcov_unmap().
> +	 */
> +	kcov_get(kcov);
> +	vma->vm_ops = &kcov_vm_ops;
> +exit:
> +	spin_unlock(&kcov->lock);
> +	vfree(area);
> +	return res;
> +}
-- 
 Kirill A. Shutemov

[toc] | [next] | [standalone]

#1311763

On Mon, Jan 18, 2016 at 1:42 PM, Kirill A. Shutemov
<kirill@shutemov.name> wrote:
> On Thu, Jan 14, 2016 at 03:22:21PM +0100, Dmitry Vyukov wrote:
>> +/*
>> + * Entry point from instrumented code.
>> + * This is called once per basic-block/edge.
>> + */
>> +void __sanitizer_cov_trace_pc(void)
>> +{
>> +     struct task_struct *t;
>> +     enum kcov_mode mode;
>> +
>> +     t = current;
>> +     /*
>> +      * We are interested in code coverage as a function of a syscall inputs,
>> +      * so we ignore code executed in interrupts.
>> +      */
>> +     if (!t || in_interrupt())
>> +             return;
>> +     mode = READ_ONCE(t->kcov_mode);
>> +     if (mode == kcov_mode_trace) {
>> +             u32 *area;
>> +             u32 pos;
>> +
>> +             /*
>> +              * There is some code that runs in interrupts but for which
>> +              * in_interrupt() returns false (e.g. preempt_schedule_irq()).
>> +              * READ_ONCE()/barrier() effectively provides load-acquire wrt
>> +              * interrupts, there are paired barrier()/WRITE_ONCE() in
>> +              * kcov_ioctl_locked().
>> +              */
>> +             barrier();
>> +             area = t->kcov_area;
>> +             /* The first u32 is number of subsequent PCs. */
>> +             pos = READ_ONCE(area[0]) + 1;
>> +             if (likely(pos < t->kcov_size)) {
>> +                     area[pos] = (u32)_RET_IP_;
>
> If area[0] is UINT32_MAX the condition will be true and you'll make
> area[0] temporary set to (u32)_RET_IP_. That's may be fun. :)

Well, if user wants to corrupt this region, he is free to do so. But
it wont't corrupt kernel, right?


>> +                     WRITE_ONCE(area[0], pos);
>> +             }
>> +     }
>> +}
>> +EXPORT_SYMBOL(__sanitizer_cov_trace_pc);
>
> ...
>
>> +static int kcov_vm_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
>> +{
>> +     struct kcov *kcov;
>> +     unsigned long off;
>> +     struct page *page;
>> +
>> +     /* Map the preallocated kcov->area. */
>> +     kcov = vma->vm_file->private_data;
>> +     off = vmf->pgoff << PAGE_SHIFT;
>> +     if (off >= kcov->size * sizeof(u32))
>> +             return VM_FAULT_SIGSEGV;
>
> SIGBUS.
>
>> +
>> +     page = vmalloc_to_page(kcov->area + off);
>> +     get_page(page);
>> +     vmf->page = page;
>> +     return 0;
>> +}
>
> BTW, since all pages pre-allocated, you don't really need fault handler --
> just setup all pages table enties during mmap. This would shorten warm up
> period for userspace. See vm_insert_page().

Done in v4
I don't need to get_page() before vm_insert_page(), right?


>> +
>> +static void kcov_unmap(struct vm_area_struct *vma)
>> +{
>> +     kcov_put(vma->vm_file->private_data);
>> +}
>> +
>> +static void kcov_map_copied(struct vm_area_struct *vma)
>> +{
>> +     struct kcov *kcov;
>> +
>> +     kcov = vma->vm_file->private_data;
>> +     kcov_get(kcov);
>> +}
>> +
>> +static const struct vm_operations_struct kcov_vm_ops = {
>> +     .fault = kcov_vm_fault,
>> +     .close = kcov_unmap,
>> +     /* Called on fork()/clone() when the mapping is copied. */
>> +     .open  = kcov_map_copied,
>> +};
>> +
>> +static int kcov_mmap(struct file *filep, struct vm_area_struct *vma)
>> +{
>> +     int res = 0;
>> +     void *area;
>> +     struct kcov *kcov = vma->vm_file->private_data;
>> +
>> +     area = vmalloc_user(vma->vm_end - vma->vm_start);
>> +     if (!area)
>> +             return -ENOMEM;
>> +
>> +     spin_lock(&kcov->lock);
>> +     if (kcov->mode == 0 || vma->vm_pgoff != 0 ||
>> +         vma->vm_end - vma->vm_start != kcov->size * sizeof(u32)) {
>
> You need VM_DONTEXPAND to keep the invariant. Not sure about VM_DONTDUMP.

Done in v4


>
>> +             res = -EINVAL;
>> +             goto exit;
>> +     }
>> +     if (!kcov->area) {
>> +             kcov->area = area;
>> +             area = NULL;
>> +     }
>> +     /*
>> +      * The file drops a reference on close, but the file
>> +      * descriptor can be closed with the mmaping still alive so we keep
>> +      * a reference for those.  This is put in kcov_unmap().
>> +      */
>> +     kcov_get(kcov);
>> +     vma->vm_ops = &kcov_vm_ops;
>> +exit:
>> +     spin_unlock(&kcov->lock);
>> +     vfree(area);
>> +     return res;
>> +}
> --
>  Kirill A. Shutemov

[toc] | [prev] | [next] | [standalone]

#1311817

On Mon, Jan 18, 2016 at 08:28:04PM +0100, Dmitry Vyukov wrote:
> On Mon, Jan 18, 2016 at 1:42 PM, Kirill A. Shutemov
> <kirill@shutemov.name> wrote:
> > On Thu, Jan 14, 2016 at 03:22:21PM +0100, Dmitry Vyukov wrote:
> >> +             area = t->kcov_area;
> >> +             /* The first u32 is number of subsequent PCs. */
> >> +             pos = READ_ONCE(area[0]) + 1;
> >> +             if (likely(pos < t->kcov_size)) {
> >> +                     area[pos] = (u32)_RET_IP_;
> >
> > If area[0] is UINT32_MAX the condition will be true and you'll make
> > area[0] temporary set to (u32)_RET_IP_. That's may be fun. :)
> 
> Well, if user wants to corrupt this region, he is free to do so. But
> it wont't corrupt kernel, right?

Right. But it's easy enough to avoid overflow too:
	"if (likely(pos && pos < t->kcov_size)"

> >> +
> >> +     page = vmalloc_to_page(kcov->area + off);
> >> +     get_page(page);
> >> +     vmf->page = page;
> >> +     return 0;
> >> +}
> >
> > BTW, since all pages pre-allocated, you don't really need fault handler --
> > just setup all pages table enties during mmap. This would shorten warm up
> > period for userspace. See vm_insert_page().
> 
> Done in v4
> I don't need to get_page() before vm_insert_page(), right?

Right. vm_insert_page() will take the referenece for you.


-- 
 Kirill A. Shutemov

[toc] | [prev] | [next] | [standalone]

#1312185

On Mon, Jan 18, 2016 at 10:52 PM, Kirill A. Shutemov
<kirill@shutemov.name> wrote:
> On Mon, Jan 18, 2016 at 08:28:04PM +0100, Dmitry Vyukov wrote:
>> On Mon, Jan 18, 2016 at 1:42 PM, Kirill A. Shutemov
>> <kirill@shutemov.name> wrote:
>> > On Thu, Jan 14, 2016 at 03:22:21PM +0100, Dmitry Vyukov wrote:
>> >> +             area = t->kcov_area;
>> >> +             /* The first u32 is number of subsequent PCs. */
>> >> +             pos = READ_ONCE(area[0]) + 1;
>> >> +             if (likely(pos < t->kcov_size)) {
>> >> +                     area[pos] = (u32)_RET_IP_;
>> >
>> > If area[0] is UINT32_MAX the condition will be true and you'll make
>> > area[0] temporary set to (u32)_RET_IP_. That's may be fun. :)
>>
>> Well, if user wants to corrupt this region, he is free to do so. But
>> it wont't corrupt kernel, right?
>
> Right. But it's easy enough to avoid overflow too:
>         "if (likely(pos && pos < t->kcov_size)"


It is easy but I don't understand what we are trying to fix.
Such check won't fix operation of kcov if user makes random memory
writes. User can also write random values which are not UINT32_MAX to
area[0]. In such case coverage will be intermixed and some pieces will
be missing.
Also this function is quite hot. A single register check and branch is
not too expensive, but it does not worth it if it does not bring any
value.


>> >> +
>> >> +     page = vmalloc_to_page(kcov->area + off);
>> >> +     get_page(page);
>> >> +     vmf->page = page;
>> >> +     return 0;
>> >> +}
>> >
>> > BTW, since all pages pre-allocated, you don't really need fault handler --
>> > just setup all pages table enties during mmap. This would shorten warm up
>> > period for userspace. See vm_insert_page().
>>
>> Done in v4
>> I don't need to get_page() before vm_insert_page(), right?
>
> Right. vm_insert_page() will take the referenece for you.


ack

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web