>-----Original Message-----
>From: Joe Eykholt [mailto:[email protected]]
>Sent: Thursday, July 16, 2009 2:12 PM
>To: Ma, Steve
>Cc: [email protected]
>Subject: Re: [Open-FCoE] [PATCH] libfc: Validating SOF, EOF, SEQ_ID,
>SEQ_CNT, f_ctl for response frames
>
>Ma, Steve wrote:
>>
>>> -----Original Message-----
>>> From: Joe Eykholt [mailto:[email protected]]
>>> Sent: Wednesday, July 15, 2009 2:40 PM
>>> To: Ma, Steve
>>> Cc: [email protected]
>>> 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 <[email protected]>
>>>> ---
>>>>
>>>> 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.
>
>Good point. I forgot about continuously increasing sequence count.
>
>>> 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.
>
>OK, sure. Still, we can get reordering, so checking sequence count is a
>problem.
>I think it's better to let the upper level (FCP or fc_disc or fcping)
>handle it.
Here I just want to point out there are other cases to get multiple response
frames. I have consulted the entire team and Chris has already made nice
comment. I will resubmit the patch based on all the input I got so far.
>>>> + (!(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
>>>> [email protected]
>>>> http://www.open-fcoe.org/mailman/listinfo/devel
>>
_______________________________________________
devel mailing list
[email protected]
http://www.open-fcoe.org/mailman/listinfo/devel
