[PATCH] ARM/hw_breakpoint: modify dead code for breakpoint_handler()
From: Wang Xu In perf_event_alloc(), event->overflow_handler is initialized to a non-null value, which makes enable_single_step(bp, addr) in breakpoint_handler() never be executed. As a matter of fact, the branch condition has been updated to is_default_overflow_handler(). Signed-off-by: Wang Xu --- arch/arm/kernel/hw_breakpoint.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/arm/kernel/hw_breakpoint.c b/arch/arm/kernel/hw_breakpoint.c index b0c195e..586a587 100644 --- a/arch/arm/kernel/hw_breakpoint.c +++ b/arch/arm/kernel/hw_breakpoint.c @@ -822,7 +822,7 @@ static void breakpoint_handler(unsigned long unknown, struct pt_regs *regs) info->trigger = addr; pr_debug("breakpoint fired: address = 0x%x\n", addr); perf_bp_event(bp, regs); - if (!bp->overflow_handler) + if (is_default_overflow_handler(bp)) enable_single_step(bp, addr); goto unlock; } -- 1.8.5.6
RE: [PATCH] sample/hw_breakpoint: avoid sample hw_breakpoint recursion for arm/arm64
-Original Message- From: Peter Zijlstra [mailto:pet...@infradead.org] Sent: Thursday, September 26, 2019 5:14 PM To: wangxu (AE) Cc: mi...@redhat.com; a...@kernel.org; mark.rutl...@arm.com; alexander.shish...@linux.intel.com; namhy...@kernel.org; gre...@linuxfoundation.org; t...@linutronix.de; rfont...@redhat.com; alli...@lohutok.net; linux-kernel@vger.kernel.org Subject: Re: [PATCH] sample/hw_breakpoint: avoid sample hw_breakpoint recursion for arm/arm64 On Mon, Sep 23, 2019 at 04:09:35PM +0800, wangxu wrote: > From: Wang Xu > > For x86/ppc, hw_breakpoint is triggered after the instruction is > executed. > > For arm/arm64, which is triggered before the instruction executed. > Arm/arm64 skips the instruction by using single step. But it only > supports default overflow_handler. Where is the recusion.. ? For arm/arm64, hw_breakpoint is triggered before the instruction executed. When instruction_A is triggered, watchpoint_handler() will deal with this exception, and after return instruction_A will be triggerd ... One using samples/hw_breakpoint/data_breakpoint.c in arm/arm64 will meet this problem. > This patch provides a chance to avoid sample hw_breakpoint recursion > for arm/arm64 by adding 'struct perf_event_attr.bp_step'. This patch also lacks justification for why this needs to come with ABI changes. There is also a distinct lack of comments. I agree too. but have no better idea... This problem is really a big pit, especially for one not familiar with implementation differences in hw breakpoint for different architectures. > Signed-off-by: Wang Xu > --- > include/linux/perf_event.h | 3 +++ > include/uapi/linux/perf_event.h | 3 ++- > samples/hw_breakpoint/data_breakpoint.c | 1 + > 3 files changed, 6 insertions(+), 1 deletion(-) > > diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h > index 61448c1..f270eb7 100644 > --- a/include/linux/perf_event.h > +++ b/include/linux/perf_event.h > @@ -1024,6 +1024,9 @@ extern int perf_event_output(struct perf_event *event, > return true; > if (unlikely(event->overflow_handler == perf_event_output_backward)) > return true; > + /* avoid sample hw_breakpoint recursion */ > + if (unlikely(event->attr.bp_step)) > + return true; This is just _wrong_.. it says that every event with bp_step set always is a 'default overflow handler', irrespective of what the overflow handler actually is. Thanks for comments. Function is_default_overflow_handler() was introduced in 1879445dfa7bbd6fe21b09c5cc72f4934798afed , which is only be called in arch/arm[64]/kernel/hw_breakpoint.c, and will never be used in other arch/ (I think). But keeping is_default_overflow_handler() unchanged, changing ' if (is_default_overflow_handler(bp)) ' to ' if (is_default_overflow_handler(bp) || unlikely(event->attr.bp_step) ) ' will be better in arch/arm[64]/kernel/hw_breakpoint.c. > return false; > } >
RE: [PATCH] vhost: It's better to use size_t for the 3rd parameter of vhost_exceeds_weight()
Hi Michael Thanks for your fast reply. As the following code, the 2nd branch of iov_iter_advance() does not check if i->count < size, when this happens, i->count -= size may cause len exceed INT_MAX, and then total_len exceed INT_MAX. handle_tx_copy() -> get_tx_bufs(..., , ...) -> init_iov_iter() -> iov_iter_advance(iter, ...) // has 3 branches: pipe_advance() // has checked the size: if (unlikely(i->count < size)) size = i->count; iov_iter_is_discard() ... // no check. iterate_and_advance() //has checked: if (unlikely(i->count < n)) n = i->count; return iov_iter_count(iter); -Original Message- From: Michael S. Tsirkin [mailto:m...@redhat.com] Sent: Monday, September 23, 2019 4:07 PM To: wangxu (AE) Cc: jasow...@redhat.com; k...@vger.kernel.org; virtualizat...@lists.linux-foundation.org; net...@vger.kernel.org; linux-kernel@vger.kernel.org Subject: Re: [PATCH] vhost: It's better to use size_t for the 3rd parameter of vhost_exceeds_weight() On Mon, Sep 23, 2019 at 03:46:41PM +0800, wangxu wrote: > From: Wang Xu > > Caller of vhost_exceeds_weight(..., total_len) in drivers/vhost/net.c > usually pass size_t total_len, which may be affected by rx/tx package. > > Signed-off-by: Wang Xu Puts a bit more pressure on the register file ... why do we care? Is there some way that it can exceed INT_MAX? > --- > drivers/vhost/vhost.c | 4 ++-- > drivers/vhost/vhost.h | 7 --- > 2 files changed, 6 insertions(+), 5 deletions(-) > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c index > 36ca2cf..159223a 100644 > --- a/drivers/vhost/vhost.c > +++ b/drivers/vhost/vhost.c > @@ -412,7 +412,7 @@ static void vhost_dev_free_iovecs(struct vhost_dev > *dev) } > > bool vhost_exceeds_weight(struct vhost_virtqueue *vq, > - int pkts, int total_len) > + int pkts, size_t total_len) > { > struct vhost_dev *dev = vq->dev; > > @@ -454,7 +454,7 @@ static size_t vhost_get_desc_size(struct > vhost_virtqueue *vq, > > void vhost_dev_init(struct vhost_dev *dev, > struct vhost_virtqueue **vqs, int nvqs, > - int iov_limit, int weight, int byte_weight) > + int iov_limit, int weight, size_t byte_weight) > { > struct vhost_virtqueue *vq; > int i; > diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h index > e9ed272..8d80389d 100644 > --- a/drivers/vhost/vhost.h > +++ b/drivers/vhost/vhost.h > @@ -172,12 +172,13 @@ struct vhost_dev { > wait_queue_head_t wait; > int iov_limit; > int weight; > - int byte_weight; > + size_t byte_weight; > }; > This just costs extra memory, and value is never large, so I don't think this matters. > -bool vhost_exceeds_weight(struct vhost_virtqueue *vq, int pkts, int > total_len); > +bool vhost_exceeds_weight(struct vhost_virtqueue *vq, int pkts, > + size_t total_len); > void vhost_dev_init(struct vhost_dev *, struct vhost_virtqueue **vqs, > - int nvqs, int iov_limit, int weight, int byte_weight); > + int nvqs, int iov_limit, int weight, size_t byte_weight); > long vhost_dev_set_owner(struct vhost_dev *dev); bool > vhost_dev_has_owner(struct vhost_dev *dev); long > vhost_dev_check_owner(struct vhost_dev *); > -- > 1.8.5.6
[PATCH] sample/hw_breakpoint: avoid sample hw_breakpoint recursion for arm/arm64
From: Wang Xu For x86/ppc, hw_breakpoint is triggered after the instruction is executed. For arm/arm64, which is triggered before the instruction executed. Arm/arm64 skips the instruction by using single step. But it only supports default overflow_handler. This patch provides a chance to avoid sample hw_breakpoint recursion for arm/arm64 by adding 'struct perf_event_attr.bp_step'. Signed-off-by: Wang Xu --- include/linux/perf_event.h | 3 +++ include/uapi/linux/perf_event.h | 3 ++- samples/hw_breakpoint/data_breakpoint.c | 1 + 3 files changed, 6 insertions(+), 1 deletion(-) diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h index 61448c1..f270eb7 100644 --- a/include/linux/perf_event.h +++ b/include/linux/perf_event.h @@ -1024,6 +1024,9 @@ extern int perf_event_output(struct perf_event *event, return true; if (unlikely(event->overflow_handler == perf_event_output_backward)) return true; + /* avoid sample hw_breakpoint recursion */ + if (unlikely(event->attr.bp_step)) + return true; return false; } diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h index bb7b271..95b81c2 100644 --- a/include/uapi/linux/perf_event.h +++ b/include/uapi/linux/perf_event.h @@ -375,7 +375,8 @@ struct perf_event_attr { ksymbol: 1, /* include ksymbol events */ bpf_event : 1, /* include bpf events */ aux_output : 1, /* generate AUX records instead of events */ - __reserved_1 : 32; + bp_step: 1, + __reserved_1 : 31; union { __u32 wakeup_events;/* wakeup every n events */ diff --git a/samples/hw_breakpoint/data_breakpoint.c b/samples/hw_breakpoint/data_breakpoint.c index c585047..9fe1351 100644 --- a/samples/hw_breakpoint/data_breakpoint.c +++ b/samples/hw_breakpoint/data_breakpoint.c @@ -46,6 +46,7 @@ static int __init hw_break_module_init(void) attr.bp_addr = kallsyms_lookup_name(ksym_name); attr.bp_len = HW_BREAKPOINT_LEN_4; attr.bp_type = HW_BREAKPOINT_W | HW_BREAKPOINT_R; + attr.bp_step = true; sample_hbp = register_wide_hw_breakpoint(, sample_hbp_handler, NULL); if (IS_ERR((void __force *)sample_hbp)) { -- 1.8.5.6
[PATCH] vhost: It's better to use size_t for the 3rd parameter of vhost_exceeds_weight()
From: Wang Xu Caller of vhost_exceeds_weight(..., total_len) in drivers/vhost/net.c usually pass size_t total_len, which may be affected by rx/tx package. Signed-off-by: Wang Xu --- drivers/vhost/vhost.c | 4 ++-- drivers/vhost/vhost.h | 7 --- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c index 36ca2cf..159223a 100644 --- a/drivers/vhost/vhost.c +++ b/drivers/vhost/vhost.c @@ -412,7 +412,7 @@ static void vhost_dev_free_iovecs(struct vhost_dev *dev) } bool vhost_exceeds_weight(struct vhost_virtqueue *vq, - int pkts, int total_len) + int pkts, size_t total_len) { struct vhost_dev *dev = vq->dev; @@ -454,7 +454,7 @@ static size_t vhost_get_desc_size(struct vhost_virtqueue *vq, void vhost_dev_init(struct vhost_dev *dev, struct vhost_virtqueue **vqs, int nvqs, - int iov_limit, int weight, int byte_weight) + int iov_limit, int weight, size_t byte_weight) { struct vhost_virtqueue *vq; int i; diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h index e9ed272..8d80389d 100644 --- a/drivers/vhost/vhost.h +++ b/drivers/vhost/vhost.h @@ -172,12 +172,13 @@ struct vhost_dev { wait_queue_head_t wait; int iov_limit; int weight; - int byte_weight; + size_t byte_weight; }; -bool vhost_exceeds_weight(struct vhost_virtqueue *vq, int pkts, int total_len); +bool vhost_exceeds_weight(struct vhost_virtqueue *vq, int pkts, + size_t total_len); void vhost_dev_init(struct vhost_dev *, struct vhost_virtqueue **vqs, - int nvqs, int iov_limit, int weight, int byte_weight); + int nvqs, int iov_limit, int weight, size_t byte_weight); long vhost_dev_set_owner(struct vhost_dev *dev); bool vhost_dev_has_owner(struct vhost_dev *dev); long vhost_dev_check_owner(struct vhost_dev *); -- 1.8.5.6