All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Allen Hubbe" <Allen.Hubbe@dell.com>
To: 'Logan Gunthorpe' <logang@deltatee.com>,
	linux-ntb@googlegroups.com, linux-kernel@vger.kernel.org
Cc: 'Jon Mason' <jdmason@kudzu.us>
Subject: RE: [PATCH 2/2] ntb_hw_switchtec: Check for alignment of the buffer in mw_set_trans()
Date: Mon, 11 Dec 2017 14:17:38 -0500	[thread overview]
Message-ID: <000301d372b4$b6042100$220c6300$@dell.com> (raw)
In-Reply-To: <d06cf74e-125c-6281-9361-22bac3468251@deltatee.com>

From: Logan Gunthorpe

> mw_get_align doesn't communicate the fact that the buffer has to be
> aligned by its size.

Is that not the purpose of the addr_align out parameter of ntb_mw_get_align()?

> It may also be that all hardware does not have this
> restriction (ie. if the hardware adds to the base address instead of
> just replacing the lower bits).
> 
> There is definitely a need to print this error somewhere as I hit this
> case and it caused very weird behavior. It was a huge pain to debug.
> Also, it's a security issue and huge bug if we end up mapping the memory
> we didn't think we were mapping.

Of course the driver should validate its parameters not allow bad mappings.  I was only commenting on the dev_err() message to the console.

What makes best sense for client drivers with regards to ntb api changes is a fair argument.  Let's see what others say.

> I don't think it's a good idea for us
> to require clients to check this as that requires a number of checks and
> a client author may forget to add it to their driver. I'd maybe go with
> a check in ntb_mw_set_trans before calling the driver, but that only
> makes sense if all hardware has the same requirement.
> 
> Logan


WARNING: multiple messages have this Message-ID (diff)
From: "Allen Hubbe" <Allen.Hubbe@dell.com>
To: "'Logan Gunthorpe'" <logang@deltatee.com>,
	<linux-ntb@googlegroups.com>, <linux-kernel@vger.kernel.org>
Cc: "'Jon Mason'" <jdmason@kudzu.us>
Subject: RE: [PATCH 2/2] ntb_hw_switchtec: Check for alignment of the buffer in mw_set_trans()
Date: Mon, 11 Dec 2017 14:17:38 -0500	[thread overview]
Message-ID: <000301d372b4$b6042100$220c6300$@dell.com> (raw)
In-Reply-To: <d06cf74e-125c-6281-9361-22bac3468251@deltatee.com>

From: Logan Gunthorpe

> mw_get_align doesn't communicate the fact that the buffer has to be
> aligned by its size.

Is that not the purpose of the addr_align out parameter of ntb_mw_get_align()?

> It may also be that all hardware does not have this
> restriction (ie. if the hardware adds to the base address instead of
> just replacing the lower bits).
> 
> There is definitely a need to print this error somewhere as I hit this
> case and it caused very weird behavior. It was a huge pain to debug.
> Also, it's a security issue and huge bug if we end up mapping the memory
> we didn't think we were mapping.

Of course the driver should validate its parameters not allow bad mappings.  I was only commenting on the dev_err() message to the console.

What makes best sense for client drivers with regards to ntb api changes is a fair argument.  Let's see what others say.

> I don't think it's a good idea for us
> to require clients to check this as that requires a number of checks and
> a client author may forget to add it to their driver. I'd maybe go with
> a check in ntb_mw_set_trans before calling the driver, but that only
> makes sense if all hardware has the same requirement.
> 
> Logan

  reply	other threads:[~2017-12-11 19:18 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-12-09  0:02 [PATCH 1/2] ntb_transport: Fix bug with max_mw_size parameter Logan Gunthorpe
2017-12-09  0:02 ` [PATCH 2/2] ntb_hw_switchtec: Check for alignment of the buffer in mw_set_trans() Logan Gunthorpe
2017-12-11 14:58   ` Allen Hubbe
2017-12-11 14:58     ` Allen Hubbe
2017-12-11 17:14     ` Logan Gunthorpe
2017-12-11 19:17       ` Allen Hubbe [this message]
2017-12-11 19:17         ` Allen Hubbe
2017-12-11 19:28         ` Logan Gunthorpe
2017-12-11 20:06           ` Allen Hubbe
2017-12-11 20:06             ` Allen Hubbe
2017-12-11 20:38             ` Logan Gunthorpe
2017-12-11 14:55 ` [PATCH 1/2] ntb_transport: Fix bug with max_mw_size parameter Allen Hubbe
2017-12-11 14:55   ` Allen Hubbe

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='000301d372b4$b6042100$220c6300$@dell.com' \
    --to=allen.hubbe@dell.com \
    --cc=jdmason@kudzu.us \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-ntb@googlegroups.com \
    --cc=logang@deltatee.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.