diff mbox

ata: fixes kernel crash while tracing ata_eh_link_autopsy event

Message ID 1508926976-11467-1-git-send-email-rsahu@apm.com (mailing list archive)
State Not Applicable
Headers show

Commit Message

Rameshwar Prasad Sahu Oct. 25, 2017, 10:22 a.m. UTC
When tracing ata link error event, the kernel crashes when the disk is
removed due to NULL pointer access by trace_ata_eh_link_autopsy API.
This occurs as the dev is NULL when the disk disappeared. Given that
the trace routine requires only the link info, pass the link info instead
of passing dev pointer to fix this kernel crash.

Signed-off-by: Rameshwar Prasad Sahu <rsahu@apm.com>
---
 drivers/ata/libata-eh.c       |    2 +-
 include/trace/events/libata.h |    9 +++++----
 2 files changed, 6 insertions(+), 5 deletions(-)

--
1.7.1

Comments

Tejun Heo Oct. 25, 2017, 2:54 p.m. UTC | #1
Hello,

On Wed, Oct 25, 2017 at 03:52:56PM +0530, Rameshwar Prasad Sahu wrote:
> @@ -288,8 +289,8 @@
>  	),
> 
>  	TP_fast_assign(
> -		__entry->ata_port	= dev->link->ap->print_id;
> -		__entry->ata_dev	= dev->link->pmp + dev->devno;
> +		__entry->ata_port	= link->ap->print_id;
> +		__entry->ata_dev	= link->pmp + link->device->devno;

The above is wrong if there are multiple devices on the link.  It
probably should take both link and dev and use dev iff it's not NULL.

Thanks.
Tejun Heo Nov. 1, 2017, 2:30 p.m. UTC | #2
Hello,

On Tue, Oct 31, 2017 at 08:52:44PM +0530, Rameshwar Sahu wrote:
> > probably should take both link and dev and use dev iff it's not NULL.
> >
> 
> Instead of this would it be better to call trace_ata_eh_link_autopsy() if
> dev is not NULL from ata error handler ??

Oh yeah, that'd work too and be probably better.

Thanks.
Rameshwar Prasad Sahu Nov. 2, 2017, 9:57 a.m. UTC | #3
On Wed, Nov 1, 2017 at 8:00 PM, Tejun Heo <tj@kernel.org> wrote:
> Hello,
>
> On Tue, Oct 31, 2017 at 08:52:44PM +0530, Rameshwar Sahu wrote:
>> > probably should take both link and dev and use dev iff it's not NULL.
>> >
>>
>> Instead of this would it be better to call trace_ata_eh_link_autopsy() if
>> dev is not NULL from ata error handler ??
>
> Oh yeah, that'd work too and be probably better.
I will post another  version with this fix.
>
> Thanks.
>
> --
> tejun
diff mbox

Patch

diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index e4effef..ab50e7d 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -2265,7 +2265,7 @@  static void ata_eh_link_autopsy(struct ata_link *link)
 			eflags |= ATA_EFLAG_DUBIOUS_XFER;
 		ehc->i.action |= ata_eh_speed_down(dev, eflags, all_err_mask);
 	}
-	trace_ata_eh_link_autopsy(dev, ehc->i.action, all_err_mask);
+	trace_ata_eh_link_autopsy(link, ehc->i.action, all_err_mask);
 	DPRINTK("EXIT\n");
 }

diff --git a/include/trace/events/libata.h b/include/trace/events/libata.h
index 2fbbf99..eff035e 100644
--- a/include/trace/events/libata.h
+++ b/include/trace/events/libata.h
@@ -276,9 +276,10 @@ 

 TRACE_EVENT(ata_eh_link_autopsy,

-	TP_PROTO(struct ata_device *dev, unsigned int eh_action, unsigned int eh_err_mask),
+	TP_PROTO(struct ata_link *link, unsigned int eh_action,
+		 unsigned int eh_err_mask),

-	TP_ARGS(dev, eh_action, eh_err_mask),
+	TP_ARGS(link, eh_action, eh_err_mask),

 	TP_STRUCT__entry(
 		__field( unsigned int,	ata_port )
@@ -288,8 +289,8 @@ 
 	),

 	TP_fast_assign(
-		__entry->ata_port	= dev->link->ap->print_id;
-		__entry->ata_dev	= dev->link->pmp + dev->devno;
+		__entry->ata_port	= link->ap->print_id;
+		__entry->ata_dev	= link->pmp + link->device->devno;
 		__entry->eh_action	= eh_action;
 		__entry->eh_err_mask	= eh_err_mask;
 	),