[Open-FCoE] [PATCH] libfc: Validating SOF, EOF, SEQ_ID, SEQ_CNT, f_ctl for response frames

Ma, Steve steve.ma at intel.com
Thu Jul 16 20:44:07 UTC 2009



>-----Original Message-----
>From: Joe Eykholt [mailto:jeykholt at cisco.com]
>Sent: Wednesday, July 15, 2009 2:40 PM
>To: Ma, Steve
>Cc: devel at open-fcoe.org
>Subject: Re: [Open-FCoE] [PATCH] libfc: Validating SOF, EOF, SEQ_ID,
>SEQ_CNT, f_ctl for response frames
>
>Steve Ma wrote:
>> This patch is for handling the received frames where the other end
>> is originating the sequence in response to the exchange originated
>> by the initiator.
>>
>> The current code is weak in validation of SOF,EOF,SEQ_ID,SEQ_CNT,f_ctl
>> for response frames, or no validations for SEQ_CNT.
>>
>> This patch is to add code to perform the validations of f_ctl in the
>> frame header for FC_FC_LAST_SEQ, FC_FC_END_SEQ, and FC_FC_SEQ_INIT
>> bits. The frame will be dropped (i.e. freed) if it fails the validation.
>>
>> Signed-off-by: Steve Ma <steve.ma at intel.com>
>> ---
>>
>>  drivers/scsi/libfc/fc_exch.c |   59
>++++++++++++++++++++++++++++++++++++------
>>  1 files changed, 51 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/scsi/libfc/fc_exch.c b/drivers/scsi/libfc/fc_exch.c
>> index f42695e..d86eda8 100644
>> --- a/drivers/scsi/libfc/fc_exch.c
>> +++ b/drivers/scsi/libfc/fc_exch.c
>> @@ -1124,7 +1124,9 @@ static void fc_exch_recv_seq_resp(struct
>fc_exch_mgr *mp, struct fc_frame *fp)
>>  	struct fc_seq *sp;
>>  	struct fc_exch *ep;
>>  	enum fc_sof sof;
>> +	enum fc_eof eof;
>>  	u32 f_ctl;
>> +	u16 cnt;
>>  	void (*resp)(struct fc_seq *, struct fc_frame *fp, void *arg);
>>  	void *ex_resp_arg;
>>  	int rc;
>> @@ -1149,29 +1151,70 @@ static void fc_exch_recv_seq_resp(struct
>fc_exch_mgr *mp, struct fc_frame *fp)
>>  		atomic_inc(&mp->stats.xid_not_found);
>>  		goto rel;
>>  	}
>> +
>>  	sof = fr_sof(fp);
>> -	if (fc_sof_is_init(sof)) {
>> +	eof = fr_eof(fp);
>> +	cnt = ntohs(fh->fh_seq_cnt);
>> +	f_ctl = ntoh24(fh->fh_f_ctl);
>> +
>> +	if ((sof ==  FC_SOF_I3 && eof == FC_EOF_T)) {
>
>Remove extra parens and extra space after the first ==.
>
>	if (sof == FC_SOF_I3 && eof == FC_EOF_T) {
>
>> +		/* first and last frame in a sequence */
>>  		sp = fc_seq_start_next(&ep->seq);
>> +		if ((cnt && (cnt != sp->cnt + 1)) ||
>> +		    (!(f_ctl & FC_FC_END_SEQ))) {
>
>Since it's the only frame of the sequence, shouldn't cnt be zero?

Because the sequence may not be the first sequence of the exchange, the SEQ_CNT can be either zero or continuously increasing. FC-FS-2, 9.10.

>
>		if (!cnt || !(f_ctl & FC_FC_END_SEQ)) {
>
>Avoid extra parens.
>
>> +			atomic_inc(&mp->stats.seq_not_found);
>> +			goto rel;
>> +		}
>> +		sp->cnt = cnt;
>>  		sp->id = fh->fh_seq_id;
>>  		sp->ssb_stat |= SSB_ST_RESP;
>> -	} else {
>> +		ep->esb_stat |= ESB_ST_SEQ_INIT;
>> +	} else if ((sof ==  FC_SOF_I3 && eof == FC_EOF_N)) {
>
>remove extra parens and extra space after first ==
>
>> +		/* first and not last frame in a sequence */
>> +		sp = fc_seq_start_next(&ep->seq);
>> +		if ((cnt && (cnt != sp->cnt + 1)) ||
>
>Again, shouldn't cnt be zero?
>
Same as above.

>> +		    (f_ctl & (FC_FC_LAST_SEQ | FC_FC_END_SEQ)) ||
>
>LAST_SEQ is allowed on any frame in the last sequence, although
>it is only required on the last frame.  So, don't check that here.
>See FC-FS-2, section 9.7.5.
>
>> +		    (f_ctl & FC_FC_SEQ_INIT)) {
>> +			atomic_inc(&mp->stats.seq_not_found);
>> +			goto rel;
>> +		}
>> +		sp->cnt = cnt;
>> +		sp->id = fh->fh_seq_id;
>> +		sp->ssb_stat |= SSB_ST_RESP;
>> +		ep->esb_stat |= ESB_ST_SEQ_INIT;
>> +	} else if ((sof ==  FC_SOF_N3) && (eof == FC_EOF_N)) {
>> +		/* middle frame in a sequence */
>>  		sp = &ep->seq;
>> -		if (sp->id != fh->fh_seq_id) {
>> +		if ((sp->id != fh->fh_seq_id) ||
>> +		    (sp->cnt + 1 != cnt) ||
>> +		    (f_ctl & (FC_FC_LAST_SEQ | FC_FC_END_SEQ)) ||
>> +		    (f_ctl & FC_FC_SEQ_INIT)) {
>
>Don't check LAST_SEQ here, either.
>Suggest removing extra parens around comparisons.
>
>> +			atomic_inc(&mp->stats.seq_not_found);
>> +			goto rel;
>> +		}
>> +		sp->cnt++;
>> +	} else if ((sof ==  FC_SOF_N3) && (eof == FC_EOF_T)) {
>> +		/* last frame in a sequence */
>> +		sp = &ep->seq;
>> +		if ((sp->id != fh->fh_seq_id) ||
>> +		    (sp->cnt + 1 != cnt) ||
>
>We do see reordering of frames for FCP when interrupt
>migration moves the receive interrupt to another CPU.
>So the sequence comparison shouldn't be done unless we somehow
>figure out how to preserve order in this case.
>
>BTW, the only non-FCP sequences that we handle as multiple frames
>are CT GPN_FT responses.  fc_disc.c already checks that they
>arrive in order, so we really don't need to check it here.
>
I have also seen multiple response frames when sending ECHO of 256 bytes to a SANBlaze target.

>> +		    (!(f_ctl & FC_FC_END_SEQ))) {
>>  			atomic_inc(&mp->stats.seq_not_found);
>>  			goto rel;
>>  		}
>> +		sp->cnt++;
>
>Need to test for SEQ_INIT here, too.
>
>> +	} else {
>> +		atomic_inc(&mp->stats.seq_not_found);
>> +		goto rel;
>>  	}
>> -	f_ctl = ntoh24(fh->fh_f_ctl);
>> -	fr_seq(fp) = sp;
>> -	if (f_ctl & FC_FC_SEQ_INIT)
>> -		ep->esb_stat |= ESB_ST_SEQ_INIT;
>>
>> +	fr_seq(fp) = sp;
>>  	if (fc_sof_needs_ack(sof))
>>  		fc_seq_send_ack(sp, fp);
>>  	resp = ep->resp;
>>  	ex_resp_arg = ep->arg;
>>
>> -	if (fh->fh_type != FC_TYPE_FCP && fr_eof(fp) == FC_EOF_T &&
>> +	if (fh->fh_type != FC_TYPE_FCP && eof == FC_EOF_T &&
>>  	    (f_ctl & (FC_FC_LAST_SEQ | FC_FC_END_SEQ)) ==
>>  	    (FC_FC_LAST_SEQ | FC_FC_END_SEQ)) {
>>  		spin_lock_bh(&ep->ex_lock);
>>
>> _______________________________________________
>> devel mailing list
>> devel at open-fcoe.org
>> http://www.open-fcoe.org/mailman/listinfo/devel




More information about the devel mailing list