RE: [Drbd-dev] DRBD-8 trunk crashes if NegDReply received in reply toRSDataRequest

RE: [Drbd-dev] DRBD-8 trunk crashes if NegDReply received in reply toRSDataRequest

[Graham, Simon]

And another one -- receive_DataRequest calls drbd_alloc_ee _before_
testing if it has a local disk; this craps out in bio_add_page because
bi_bdev is NULL (I think -- it's a bit hard to read the optimized
disassembly) -- certainly, when we go diskless because of an error,
after_state_ch frees the drbd_backing_dev and sets the bc pointer in
mdev to NULL -- a data request received after that will call
drbd_alloc_ee which will do the wrong thing.

I think the fix is to move the test for having good data to before the
call to drbd_alloc_ee; does that seem reasonable?

/simgr


> -----Original Message-----
> From: drbd-dev-bounces@linbit.com [mailto:drbd-dev-bounces@linbit.com]
> On Behalf Of Graham, Simon
> Sent: Saturday, August 05, 2006 10:29 PM
> To: drbd-dev@linbit.com
> Subject: [Drbd-dev] DRBD-8 trunk crashes if NegDReply received in
reply
> toRSDataRequest
> 
> When an RSDataRequest is sent, the block_id field in the request is
set
> to SYNCER_ID (-1) - if the receiver is unable to process the request
> (e.g. if the receiver doesn't have good data) then it sends back a
> NegDReply with the same block_id - on the origin side, got_NegDReply
> attempts to validate the block_id value using drbd_pr_verify which
> promptly crashes attempting to reference the master_bio field to get
> the
> sector (drbd_req_get_sector()).
> 
> I found this testing my fixes for removing panic() calls on meta data
> read/write failures but I thought it was worth bugging separately;
> clearly this routine needs to validate the request pointer before
> attempting to access it at all, but also we should have got a
> NedRSDreply in this case - so there are at least two bugs here:
> 
> 1. The target side should have send a NegRSDreply in this case;
> receive_DataRequest should switch on h->command
>    to decide what response to send when it bails early
> 2. drbd_pr_verify should NOT call drbd_req_get_sector() before
> validating the pointer - is there any reason why
>    it shouldn't use the sector value passed in as a parameter?
> 
> I can make patches for these when we agree on the right solution...
> 
> Simon
> 
> _______________________________________________
> drbd-dev mailing list
> drbd-dev@lists.linbit.com
> http://lists.linbit.com/mailman/listinfo/drbd-dev

RE: [Drbd-dev] DRBD-8 trunk crashes if NegDReply received in reply toRSDataRequest

[Graham, Simon]

Oh wait... that wont work -- the call to send the negative ack takes the
epoch_entry* as a parameter...

How about having a new routine that can send a negative ack based on a
Data packet pointer? I don't like the idea of making drbd_aloc_ee able
to handle this case as you will get a partially initialized epoch_entry*
back in that case...

/simgr

> -----Original Message-----
> From: Graham, Simon
> Sent: Sunday, August 06, 2006 12:14 PM
> To: Graham, Simon; drbd-dev@linbit.com
> Subject: RE: [Drbd-dev] DRBD-8 trunk crashes if NegDReply received in
> reply toRSDataRequest
> 
> And another one -- receive_DataRequest calls drbd_alloc_ee _before_
> testing if it has a local disk; this craps out in bio_add_page because
> bi_bdev is NULL (I think -- it's a bit hard to read the optimized
> disassembly) -- certainly, when we go diskless because of an error,
> after_state_ch frees the drbd_backing_dev and sets the bc pointer in
> mdev to NULL -- a data request received after that will call
> drbd_alloc_ee which will do the wrong thing.
> 
> I think the fix is to move the test for having good data to before the
> call to drbd_alloc_ee; does that seem reasonable?
> 
> /simgr
> 
> 
> > -----Original Message-----
> > From: drbd-dev-bounces@linbit.com [mailto:drbd-dev-
> bounces@linbit.com]
> > On Behalf Of Graham, Simon
> > Sent: Saturday, August 05, 2006 10:29 PM
> > To: drbd-dev@linbit.com
> > Subject: [Drbd-dev] DRBD-8 trunk crashes if NegDReply received in
> reply
> > toRSDataRequest
> >
> > When an RSDataRequest is sent, the block_id field in the request is
> set
> > to SYNCER_ID (-1) - if the receiver is unable to process the request
> > (e.g. if the receiver doesn't have good data) then it sends back a
> > NegDReply with the same block_id - on the origin side, got_NegDReply
> > attempts to validate the block_id value using drbd_pr_verify which
> > promptly crashes attempting to reference the master_bio field to get
> > the
> > sector (drbd_req_get_sector()).
> >
> > I found this testing my fixes for removing panic() calls on meta
data
> > read/write failures but I thought it was worth bugging separately;
> > clearly this routine needs to validate the request pointer before
> > attempting to access it at all, but also we should have got a
> > NedRSDreply in this case - so there are at least two bugs here:
> >
> > 1. The target side should have send a NegRSDreply in this case;
> > receive_DataRequest should switch on h->command
> >    to decide what response to send when it bails early
> > 2. drbd_pr_verify should NOT call drbd_req_get_sector() before
> > validating the pointer - is there any reason why
> >    it shouldn't use the sector value passed in as a parameter?
> >
> > I can make patches for these when we agree on the right solution...
> >
> > Simon
> >
> > _______________________________________________
> > drbd-dev mailing list
> > drbd-dev@lists.linbit.com
> > http://lists.linbit.com/mailman/listinfo/drbd-dev

RE: [Drbd-dev] DRBD-8 trunk crashes if NegDReply received in reply toRSDataRequest

[Graham, Simon]

> I did the patching myself, in that cases. Please verify my
> changes:
> http://lists.linbit.com/pipermail/drbd-cvs/2006-August/001179.html
> 

Patches look good - thanks,
Simon

Re: [Drbd-dev] DRBD-8 trunk crashes if NegDReply received in reply toRSDataRequest

[Philipp Reisner]

Am Sonntag, 6. August 2006 18:18 schrieb Graham, Simon:
> Oh wait... that wont work -- the call to send the negative ack takes the
> epoch_entry* as a parameter...
>
> How about having a new routine that can send a negative ack based on a
> Data packet pointer? I don't like the idea of making drbd_aloc_ee able
> to handle this case as you will get a partially initialized epoch_entry*
> back in that case...
>

Again, I agree completely.

In adition I looked out for more drbd_alloc_ee() and moved them after
the inc_local() statement....

Please check my patch
http://lists.linbit.com/pipermail/drbd-cvs/2006-August/001180.html

-philipp
-- 
: Dipl-Ing Philipp Reisner                      Tel +43-1-8178292-50 :
: LINBIT Information Technologies GmbH          Fax +43-1-8178292-82 :
: Schönbrunnerstr 244, 1120 Vienna, Austria    http://www.linbit.com :